Page MenuHomeFreeBSD

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

Authored by kp on Apr 8 2018, 9:08 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Jul 15, 4:41 AM
Unknown Object (File)
Tue, Jul 9, 7:10 PM
Unknown Object (File)
Sat, Jul 6, 12:15 PM
Unknown Object (File)
Sun, Jun 30, 5:17 PM
Unknown Object (File)
Sun, Jun 30, 5:17 PM
Unknown Object (File)
Sun, Jun 30, 5:17 PM
Unknown Object (File)
Sun, Jun 30, 5:16 PM
Unknown Object (File)
Sun, Jun 30, 2:50 AM
Subscribers

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 - subversion
Lint
Lint Not Applicable
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.