| Message ID | 20231127204206.3593559-1-zack@kde.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | input/vmmouse: Fix device name copies | expand |
Zack, On Mon, Nov 27, 2023 at 03:42:06PM -0500, Zack Rusin wrote: > From: Zack Rusin <zackr@vmware.com> > > Make sure vmmouse_data::phys can hold serio::phys (which is 32 bytes) > plus an extra string, extend it to 64. > > Fixes gcc13 warnings: > drivers/input/mouse/vmmouse.c: In function ‘vmmouse_init’: > drivers/input/mouse/vmmouse.c:455:53: warning: ‘/input1’ directive output may be truncated writing 7 bytes into a region of size between 1 and 32 [-Wformat-truncation=] > 455 | snprintf(priv->phys, sizeof(priv->phys), "%s/input1", > | ^~~~~~~ > drivers/input/mouse/vmmouse.c:455:9: note: ‘snprintf’ output between 8 and 39 bytes into a destination of size 32 > 455 | snprintf(priv->phys, sizeof(priv->phys), "%s/input1", > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 456 | psmouse->ps2dev.serio->phys); > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Signed-off-by: Zack Rusin <zackr@vmware.com> > Fixes: 8b8be51b4fd3 ("Input: add vmmouse driver") > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: VMware Graphics Reviewers <linux-graphics-maintainer@vmware.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Robert Jarzmik <robert.jarzmik@free.fr> > Cc: Raul Rangel <rrangel@chromium.org> > Cc: linux-input@vger.kernel.org > Cc: <stable@vger.kernel.org> # v4.1+ > --- > drivers/input/mouse/vmmouse.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/input/mouse/vmmouse.c b/drivers/input/mouse/vmmouse.c > index ea9eff7c8099..7248cada4c8c 100644 > --- a/drivers/input/mouse/vmmouse.c > +++ b/drivers/input/mouse/vmmouse.c > @@ -72,7 +72,7 @@ > */ > struct vmmouse_data { > struct input_dev *abs_dev; > - char phys[32]; > + char phys[64]; This simply wastes 32 bytes. It is perfectly fine to truncate phys (which does not happen in real life). -Wformat-truncation is disabled in normal builds, folks should stop using it with W=1 as well. Thanks.
On Sun, Dec 3, 2023, at 19:41, Dmitry Torokhov wrote: > On Mon, Nov 27, 2023 at 03:42:06PM -0500, Zack Rusin wrote: >> From: Zack Rusin <zackr@vmware.com> >> >> Make sure vmmouse_data::phys can hold serio::phys (which is 32 bytes) >> plus an extra string, extend it to 64. >> >> Fixes gcc13 warnings: >> drivers/input/mouse/vmmouse.c: In function ‘vmmouse_init’: >> drivers/input/mouse/vmmouse.c:455:53: warning: ‘/input1’ directive output may be truncated writing 7 bytes into a region of size between 1 and 32 [-Wformat-truncation=] >> 455 | snprintf(priv->phys, sizeof(priv->phys), "%s/input1", >> | ^~~~~~~ >> drivers/input/mouse/vmmouse.c:455:9: note: ‘snprintf’ output between 8 and 39 bytes into a destination of size 32 >> 455 | snprintf(priv->phys, sizeof(priv->phys), "%s/input1", >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> 456 | psmouse->ps2dev.serio->phys); >> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > This simply wastes 32 bytes. It is perfectly fine to truncate phys > (which does not happen in real life). > > -Wformat-truncation is disabled in normal builds, folks should stop > using it with W=1 as well. It does find real bugs, and we are fairly close to being able to enable it by default once the remaining warnings are all fixed. It also doesn't waste any memory in this specific case since vmmouse_data is currently at 168 bytes, which gets rounded up to either 192 or 256 bytes anyway. I'd suggest using the minimum size that is large enough though, in this case 39 bytes for the string I guess. Arnd
From: Arnd Bergmann > Sent: 03 December 2023 20:51 > On Sun, Dec 3, 2023, at 19:41, Dmitry Torokhov wrote: > > On Mon, Nov 27, 2023 at 03:42:06PM -0500, Zack Rusin wrote: > >> From: Zack Rusin <zackr@vmware.com> > >> > >> Make sure vmmouse_data::phys can hold serio::phys (which is 32 bytes) > >> plus an extra string, extend it to 64. > >> > >> Fixes gcc13 warnings: > >> drivers/input/mouse/vmmouse.c: In function ‘vmmouse_init’: > >> drivers/input/mouse/vmmouse.c:455:53: warning: ‘/input1’ directive output may be truncated writing > 7 bytes into a region of size between 1 and 32 [-Wformat-truncation=] > >> 455 | snprintf(priv->phys, sizeof(priv->phys), "%s/input1", > >> | ^~~~~~~ > >> drivers/input/mouse/vmmouse.c:455:9: note: ‘snprintf’ output between 8 and 39 bytes into a > destination of size 32 > >> 455 | snprintf(priv->phys, sizeof(priv->phys), "%s/input1", > >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > >> 456 | psmouse->ps2dev.serio->phys); > >> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > This simply wastes 32 bytes. It is perfectly fine to truncate phys > > (which does not happen in real life). > > > > -Wformat-truncation is disabled in normal builds, folks should stop > > using it with W=1 as well. > > It does find real bugs, and we are fairly close to being able > to enable it by default once the remaining warnings are all > fixed. > > It also doesn't waste any memory in this specific case since > vmmouse_data is currently at 168 bytes, which gets rounded > up to either 192 or 256 bytes anyway. I'd suggest using > the minimum size that is large enough though, in this case > 39 bytes for the string I guess. That rather depends on whether any of the earlier char[] lengths have been rounded up to a 'nice' value. I'd also have thought that dangerous overflows would come from unbounded %s formats, not fixed size strings or integers that are always small. There really ought to be a sane method of telling gcc not to bleat about snprintf() potentially overflowing the target. I've tried a few thing but none of them work. IIRC using the result (in some ways) is enough, but neither (void)snprintf(...); or if (snprintf(...)); is enough (but they 'fix' 'warn unused result'). David > > Arnd - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Sun, Dec 03, 2023 at 09:14:49PM +0000, David Laight wrote: > From: Arnd Bergmann > > Sent: 03 December 2023 20:51 > > On Sun, Dec 3, 2023, at 19:41, Dmitry Torokhov wrote: > > > On Mon, Nov 27, 2023 at 03:42:06PM -0500, Zack Rusin wrote: > > >> From: Zack Rusin <zackr@vmware.com> > > >> > > >> Make sure vmmouse_data::phys can hold serio::phys (which is 32 bytes) > > >> plus an extra string, extend it to 64. > > >> > > >> Fixes gcc13 warnings: > > >> drivers/input/mouse/vmmouse.c: In function ‘vmmouse_init’: > > >> drivers/input/mouse/vmmouse.c:455:53: warning: ‘/input1’ directive output may be truncated writing > > 7 bytes into a region of size between 1 and 32 [-Wformat-truncation=] > > >> 455 | snprintf(priv->phys, sizeof(priv->phys), "%s/input1", > > >> | ^~~~~~~ > > >> drivers/input/mouse/vmmouse.c:455:9: note: ‘snprintf’ output between 8 and 39 bytes into a > > destination of size 32 > > >> 455 | snprintf(priv->phys, sizeof(priv->phys), "%s/input1", > > >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > >> 456 | psmouse->ps2dev.serio->phys); > > >> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > This simply wastes 32 bytes. It is perfectly fine to truncate phys > > > (which does not happen in real life). > > > > > > -Wformat-truncation is disabled in normal builds, folks should stop > > > using it with W=1 as well. > > > > It does find real bugs, and we are fairly close to being able > > to enable it by default once the remaining warnings are all > > fixed. > > > > It also doesn't waste any memory ... at this time ... > > in this specific case since > > vmmouse_data is currently at 168 bytes, which gets rounded > > up to either 192 or 256 bytes anyway. I'd suggest using > > the minimum size that is large enough though, in this case > > 39 bytes for the string I guess. This assumes we never change how our allocators work to provide better memory packing. > > That rather depends on whether any of the earlier char[] lengths > have been rounded up to a 'nice' value. > > I'd also have thought that dangerous overflows would come from > unbounded %s formats, not fixed size strings or integers that are > always small. > > There really ought to be a sane method of telling gcc not to bleat > about snprintf() potentially overflowing the target. Yes, that would be my preference before we enable this warning globally. Thanks.
diff --git a/drivers/input/mouse/vmmouse.c b/drivers/input/mouse/vmmouse.c index ea9eff7c8099..7248cada4c8c 100644 --- a/drivers/input/mouse/vmmouse.c +++ b/drivers/input/mouse/vmmouse.c @@ -72,7 +72,7 @@ */ struct vmmouse_data { struct input_dev *abs_dev; - char phys[32]; + char phys[64]; char dev_name[128]; };