Page MenuHomeFreeBSD

pf: Fix ioctls which copy out arrays
ClosedPublic

Authored by markj on Jul 26 2021, 9:26 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 10, 2:32 AM
Unknown Object (File)
Thu, Mar 28, 6:46 PM
Unknown Object (File)
Jan 29 2024, 7:00 PM
Unknown Object (File)
Jan 16 2024, 9:51 PM
Unknown Object (File)
Dec 23 2023, 3:23 AM
Unknown Object (File)
Dec 12 2023, 7:43 AM
Unknown Object (File)
Dec 10 2023, 9:19 AM
Unknown Object (File)
Dec 10 2023, 3:03 AM

Details

Summary

A number of pf ioctls which copy out arrays have the following
structure:

  • Caller specifies the size of the array
  • ioctl handler allocates a buffer to store it
  • ioctl handler populates the array, returning the actual number of items back to userland
  • ioctl handler copies the array to userland

It can happen that the caller specified a size larger than the resulting
array. In this case we were leaving some entries uninitialized but
copying them out anyway. This change modifies the ioctl handlers to
copy out only the number of array entries that we return.

Reported by: KMSAN
Sponsored by: The FreeBSD Foundation

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Consistently use braces around multiline statements.

This is clearly an improvement, but I think it doesn't cover everything.

All these call a function that can still fail to copy data into the buffer. If the provided size is too small they'll only update the size pointer and return 0. They will not write to the buffer. So we can still end up copying out an uninitialised array. (except for pfi_get_ifaces())
The easiest fix would be to add M_ZERO to the mallocarray() calls.

In D31313#705343, @kp wrote:

This is clearly an improvement, but I think it doesn't cover everything.

All these call a function that can still fail to copy data into the buffer. If the provided size is too small they'll only update the size pointer and return 0. They will not write to the buffer. So we can still end up copying out an uninitialised array. (except for pfi_get_ifaces())
The easiest fix would be to add M_ZERO to the mallocarray() calls.

Ah, I hadn't noticed this. It's kind of strange that we copy out even when we have nothing to copy.I did it this way partly to avoid needless zeroing of arrays that we're going to overwrite anyway, but it's probably best to just keep things simple and zero the arrays.

Simply zero arrays that get initialized by the kernel before being
copied out.

This revision is now accepted and ready to land.Jul 27 2021, 8:43 PM