[ +CC: Arnd ]
On Wed, Apr 20, 2022 at 11:11:26AM -0700, Kees Cook wrote:
On Wed, Apr 20, 2022 at 02:33:06PM +0200, Jann Horn wrote:
On Wed, Apr 20, 2022 at 10:14 AM Johan Hovold johan@kernel.org wrote:
On Mon, Apr 18, 2022 at 09:17:42PM -0700, Kees Cook wrote:
This looks like it's harmless, as both the source and the destinations are currently the same allocation size (4 bytes) and don't use their padding, but if anything were to ever be added after the "mcr" member in "struct whiteheat_private", it would be overwritten. The structs both have a single u8 "mcr" member, but are 4 bytes in padded size. The memcpy() destination was explicitly targeting the u8 member (size 1) with the length of the whole structure (size 4), triggering the memcpy buffer overflow warning:
Ehh... No. The size of a structure with a single u8 is 1, not 4. There's nothing wrong with the current code even if the use of memcpy for this is a bit odd.
I thought that was surprising too, and suspected it was something specific to the build (as Jann also suggested). I tracked it down[1] to "-mabi=apcs-gnu", which is from CONFIG_AEABI=n.
whiteheat_private { __u8 mcr; /* 0 1 */
/* size: 4, cachelines: 1, members: 1 */ /* padding: 3 */ /* last cacheline: 4 bytes */};
I stand corrected, thanks.
Do we have other ABIs that can increase the alignment of structures like this?
Skimming lore reveals a few subsystems that have started depending on !OABI to not have to deal with this. Apparently the old ARM ABI is deprecated in user space since gcc-4.6:
https://lore.kernel.org/all/20190304193723.657089-1-arnd@arndb.de/
Perhaps time to drop support from the kernel too?
Given nothing actually uses "struct whiteheat_dr_info", except for the sizing (har har), I suspect the better solution is just to do:
info->mcr = command_info->result_buffer[0];
Yeah, that works for now. Ideally, we'd cast the result buffer to struct whiteheat_dr_info and access its single field. But that's not what current code does and the above is no less confusing.
Patch applied, thanks.
Johan