Page MenuHomeFreeBSD

re: Avoid subobject overread when setting IDRn
ClosedPublic

Authored by jrtc27 on Dec 22 2021, 5:42 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 7, 6:35 PM
Unknown Object (File)
Wed, Oct 16, 10:30 AM
Unknown Object (File)
Wed, Oct 16, 8:10 AM
Unknown Object (File)
Sep 24 2024, 6:50 PM
Unknown Object (File)
Sep 24 2024, 12:40 PM
Unknown Object (File)
Sep 24 2024, 11:39 AM
Unknown Object (File)
Sep 23 2024, 11:38 PM
Unknown Object (File)
Sep 20 2024, 7:53 AM
Subscribers

Details

Summary

IDR0-IDR5 can be read byte-by-byte but must be written to as 4-byte
words. The current code to do this is rather clunky and ends up reading
past the end of the union's eaddr member due to MAC addresses only being
6 bytes. In practice this ends up being fine because the align_dummy
member will pad the union to a multiple of 4 bytes, but this is dodgy,
and on CHERI with subobject bounds enforcement enabled, as is done in
CheriBSD's pure-capability kernel, will trap.

Instead, make the buffer in use the right size, just use an array of
uint32_t's rather than a char buffer that's then cast to uint32_t * to
simplify it in the process, and zero-initialise it first to avoid
reading uninitialised data in the trailing bytes.

Found by: CHERI

Diff Detail

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

Event Timeline

sys/dev/re/if_re.c
3196–3197

If you just bzero(idr, sizeof(idr)) before the bcopy, is the compiler smart enough to realize the first 6 bytes are overwritten and only zero the last two bytes? (I've kind of assumed compilers do figure that out nowadays.) If so, I think doing that might be easier to read.

sys/dev/re/if_re.c
3196–3197

If the pointer to the buffer leaks it seems Clang does get a bit confused and becomes marginally worse one way round, but only by one or two instructions. If you give it a more "full" example and actually pass the contents not a pointer to the contents then Clang just turns it into what you'd hope to get, whilst GCC fails equally miserably for both ways round: https://godbolt.org/z/Ynad3z3Ma

So yeah this can just be a full bzero first.

jrtc27 edited the summary of this revision. (Show Details)

Swap bzero and bcopy order for simplicity

This revision is now accepted and ready to land.Dec 23 2021, 8:13 PM