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.
In file included from include/linux/string.h:253, from include/linux/bitmap.h:11, from include/linux/cpumask.h:12, from include/linux/smp.h:13, from include/linux/lockdep.h:14, from include/linux/spinlock.h:62, from include/linux/mmzone.h:8, from include/linux/gfp.h:6, from include/linux/slab.h:15, from drivers/usb/serial/whiteheat.c:17: In function 'fortify_memcpy_chk', inlined from 'firm_send_command' at drivers/usb/serial/whiteheat.c:587:4: include/linux/fortify-string.h:328:25: warning: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning] 328 | __write_overflow_field(p_size_field, size); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
So something is confused here.
Expand the memcpy() to the entire structure, though perhaps the correct solution is to mark all the USB command structures as "__packed".
Again no, why would you potentially overwrite the whole structure just to update a single field? This is just wrong.
And the only structure that needs a __packed which doesn't have it already is the unused struct whiteheat_dump.
Reported-by: kernel test robot lkp@intel.com Link: https://lore.kernel.org/lkml/202204142318.vDqjjSFn-lkp@intel.com Cc: Johan Hovold johan@kernel.org Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: linux-usb@vger.kernel.org Cc: stable@vger.kernel.org Signed-off-by: Kees Cook keescook@chromium.org
drivers/usb/serial/whiteheat.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/serial/whiteheat.c b/drivers/usb/serial/whiteheat.c index da65d14c9ed5..6e00498843fb 100644 --- a/drivers/usb/serial/whiteheat.c +++ b/drivers/usb/serial/whiteheat.c @@ -584,7 +584,7 @@ static int firm_send_command(struct usb_serial_port *port, __u8 command, switch (command) { case WHITEHEAT_GET_DTR_RTS: info = usb_get_serial_port_data(port);
memcpy(&info->mcr, command_info->result_buffer,
memcpy(info, command_info->result_buffer, sizeof(struct whiteheat_dr_info)); break;
Johan