| Message ID | 20230705140242.844167-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 5f151364b1da6bd217632fd4ee8cc24eaf66a497 |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: hyperv: avoid struct memcpy overrun warning | expand |
From: Arnd Bergmann <arnd@kernel.org> Sent: Wednesday, July 5, 2023 7:02 AM > > A previous patch addressed the fortified memcpy warning for most > builds, but I still see this one with gcc-9: > > In file included from include/linux/string.h:254, > from drivers/hid/hid-hyperv.c:8: > In function 'fortify_memcpy_chk', > inlined from 'mousevsc_on_receive' at drivers/hid/hid-hyperv.c:272:3: > include/linux/fortify-string.h:583:4: error: call to '__write_overflow_field' declared with > attribute warning: detected write beyond size of field (1st parameter); maybe use > struct_group()? [-Werror=attribute-warning] > 583 | __write_overflow_field(p_size_field, size); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > My guess is that the WARN_ON() itself is what confuses gcc, so it no > longer sees that there is a correct range check. Rework the code in a > way that helps readability and avoids the warning. > > Fixes: 542f25a944715 ("HID: hyperv: Replace one-element array with flexible-array member") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/hid/hid-hyperv.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c > index 49d4a26895e76..f33485d83d24f 100644 > --- a/drivers/hid/hid-hyperv.c > +++ b/drivers/hid/hid-hyperv.c > @@ -258,19 +258,17 @@ static void mousevsc_on_receive(struct hv_device *device, > > switch (hid_msg_hdr->type) { > case SYNTH_HID_PROTOCOL_RESPONSE: > + len = struct_size(pipe_msg, data, pipe_msg->size); > + > /* > * While it will be impossible for us to protect against > * malicious/buggy hypervisor/host, add a check here to > * ensure we don't corrupt memory. > */ > - if (struct_size(pipe_msg, data, pipe_msg->size) > - > sizeof(struct mousevsc_prt_msg)) { > - WARN_ON(1); > + if (WARN_ON(len > sizeof(struct mousevsc_prt_msg))) > break; > - } > > - memcpy(&input_dev->protocol_resp, pipe_msg, > - struct_size(pipe_msg, data, pipe_msg->size)); > + memcpy(&input_dev->protocol_resp, pipe_msg, len); > complete(&input_dev->wait_event); > break; > > -- > 2.39.2 Reviewed-by: Michael Kelley <mikelley@microsoft.com>
On Wed, 05 Jul 2023 16:02:24 +0200, Arnd Bergmann wrote: > A previous patch addressed the fortified memcpy warning for most > builds, but I still see this one with gcc-9: > > In file included from include/linux/string.h:254, > from drivers/hid/hid-hyperv.c:8: > In function 'fortify_memcpy_chk', > inlined from 'mousevsc_on_receive' at drivers/hid/hid-hyperv.c:272:3: > include/linux/fortify-string.h:583:4: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning] > 583 | __write_overflow_field(p_size_field, size); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git (for-6.5/upstream-fixes), thanks! [1/1] HID: hyperv: avoid struct memcpy overrun warning https://git.kernel.org/hid/hid/c/5f151364b1da Cheers,
On Wed, Jul 05, 2023 at 04:02:24PM +0200, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > A previous patch addressed the fortified memcpy warning for most > builds, but I still see this one with gcc-9: JFYI: as of today I have run Linux Next with `make W=1 allmodconfig` on x86_64 and it seems there are still tons of similar issues which break the build.
On Mon, Jul 17, 2023, at 11:36, Andy Shevchenko wrote: > On Wed, Jul 05, 2023 at 04:02:24PM +0200, Arnd Bergmann wrote: >> From: Arnd Bergmann <arnd@arndb.de> >> >> A previous patch addressed the fortified memcpy warning for most >> builds, but I still see this one with gcc-9: > > JFYI: as of today I have run Linux Next with `make W=1 allmodconfig` on x86_64 > and it seems there are still tons of similar issues which break the build. It's a bit more complex: - yes, there are lots of warnings for memcpy() read overflow when you build allmodconfig kernels with W=1. I have patches for all of these and plan to submit them over time. - This particular one is a memcpy /write/ overflow, i.e. the destination type overflows with the length according to gcc's understanding of the structures. This warning is enabled even without W=1, though it may only show up in certain configurations or compiler versions. Arnd
diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c index 49d4a26895e76..f33485d83d24f 100644 --- a/drivers/hid/hid-hyperv.c +++ b/drivers/hid/hid-hyperv.c @@ -258,19 +258,17 @@ static void mousevsc_on_receive(struct hv_device *device, switch (hid_msg_hdr->type) { case SYNTH_HID_PROTOCOL_RESPONSE: + len = struct_size(pipe_msg, data, pipe_msg->size); + /* * While it will be impossible for us to protect against * malicious/buggy hypervisor/host, add a check here to * ensure we don't corrupt memory. */ - if (struct_size(pipe_msg, data, pipe_msg->size) - > sizeof(struct mousevsc_prt_msg)) { - WARN_ON(1); + if (WARN_ON(len > sizeof(struct mousevsc_prt_msg))) break; - } - memcpy(&input_dev->protocol_resp, pipe_msg, - struct_size(pipe_msg, data, pipe_msg->size)); + memcpy(&input_dev->protocol_resp, pipe_msg, len); complete(&input_dev->wait_event); break;