Page MenuHomeFreeBSD

re: Avoid subobject overread when setting IDRn
ClosedPublic

Authored by jrtc27 on Dec 22 2021, 5:42 PM.
Tags
None
Referenced Files
F107436938: D33617.id100516.diff
Tue, Jan 14, 3:47 AM
Unknown Object (File)
Fri, Jan 10, 11:03 PM
Unknown Object (File)
Thu, Dec 26, 8:32 AM
Unknown Object (File)
Dec 14 2024, 6:49 AM
Unknown Object (File)
Dec 9 2024, 7:09 PM
Unknown Object (File)
Dec 7 2024, 11:22 AM
Unknown Object (File)
Dec 2 2024, 10:29 PM
Unknown Object (File)
Nov 29 2024, 4:18 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