pf: grab pf_config_lock when freeing a rule

[rew]

Summary

Take PF_CONFIG_LOCK() when pf_free_rule() is called. The assert being
hit was added in bd7762c86986537a5b393537b85de31b1556735b.

When adding a new rule through DIOCCHANGERULE it's possible the new rule
will be freed. This can happen if an error occurs or when the new rule
is to be added before/after a given rule number that doesn't exist.

It is the latter scenario that syzkaller found.

Reported by: [e-mail]

[mjg]

I'm aware of this crash, but this is more broken than that and sprinkling the lock only takes care of the apparent problem. The key here is that the ioctl at hand does not properly update the rule anymore and unfortunately there are no tests for it either. I'm going to fix it soon(tm).

[kp]

In D34880#790375, @mjg wrote:

I'm aware of this crash, but this is more broken than that and sprinkling the lock only takes care of the apparent problem. The key here is that the ioctl at hand does not properly update the rule anymore and unfortunately there are no tests for it either. I'm going to fix it soon(tm).

I'm very, very tempted to say we should just remove DIOCCHANGERULE, but at least miniupnpd actually uses it.

[rew]

In D34880#790376, @kp wrote:

I'm very, very tempted to say we should just remove DIOCCHANGERULE, but at least miniupnpd actually uses it.

Looks like it uses PF_CHANGE_REMOVE, PF_CHANGE_GET_TICKET, and PF_CHANGE_ADD_TAIL from the ioctl.

The locking contract did seem a bit odd when I was looking at this, guess because there's active work going on here.

I'll let this go, thanks for the comments.

[mjg]

thanks for the patch. if there was a way of marking syzkaller crashes as worked on I would have done it :)