Page MenuHomeFreeBSD

pf: Limit ioctl to a reasonable and tuneable number of elements
ClosedPublic

Authored by kp on Apr 8 2018, 9:08 PM.

Details

Summary

pf ioctls frequently take a variable number of elements as argument. This can potentially allow users to request very large allocations.
These will fail, but even a failing M_NOWAIT might tie up resources and result in concurrent M_WAITOK allocations entering vm_wait and inducing reclamation of caches.

Limit these ioctls to what should be a reasonable value, but allow users to tune it should they need to.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Looks good in general. It seems that some cases use WOULD_OVERFLOW as a secondary check and some don't. Is there any reason for that?

Maybe the tuned value could be clamped to a limit that avoids WOULD_OVERFLOW problems (SIZE_MAX / <largest item size>?), and WOULD_OVERFLOW could be dropped from each conditional.

sys/netpfil/pf/pf_ioctl.c
219 ↗(On Diff #41282)

mismatch between extern linkage here and static in definition.

This revision is now accepted and ready to land.Apr 8 2018, 9:55 PM

Add missing WOULD_OVERFLOW checks.

This revision now requires review to proceed.Apr 9 2018, 7:39 AM

Fix static / extern mismatch

In D15018#316088, @cem wrote:

Looks good in general. It seems that some cases use WOULD_OVERFLOW as a secondary check and some don't. Is there any reason for that?

Good catch, I missed it in the first two cases because that used to be a fixed 65k value. Now that it's user configurable it needs the check too.

Maybe the tuned value could be clamped to a limit that avoids WOULD_OVERFLOW problems (SIZE_MAX / <largest item size>?), and WOULD_OVERFLOW could be dropped from each conditional.

That would be possible too, but I'm a bit worried that this'll get extended to other ioctl commands and we'll forget to update the maximum value.

In D15018#316121, @kristof wrote:
In D15018#316088, @cem wrote:

Looks good in general. It seems that some cases use WOULD_OVERFLOW as a secondary check and some don't. Is there any reason for that?

Good catch, I missed it in the first two cases because that used to be a fixed 65k value. Now that it's user configurable it needs the check too.

Thanks :-)

Maybe the tuned value could be clamped to a limit that avoids WOULD_OVERFLOW problems (SIZE_MAX / <largest item size>?), and WOULD_OVERFLOW could be dropped from each conditional.

That would be possible too, but I'm a bit worried that this'll get extended to other ioctl commands and we'll forget to update the maximum value.

Yeah, makes sense.

Patch looks good to me.

This revision is now accepted and ready to land.Apr 10 2018, 4:04 PM
This revision was automatically updated to reflect the committed changes.