[PATCH v1 0/4] Input: Increase size of phys in the drivers

[PATCH v1 0/4] Input: Increase size of phys in the drivers

[Andy Shevchenko]

The drivers are using local member of 32 bytes to hold up to 40 (one-byte)
characters. GCC complains on that. This series fixes the issue in the affected
input drivers. Note, this is currently the biggest part of the warnings that
are being treated as errors with the default configurations on x86. With this
being applied we become quite close to enable CONFIG_WERROR=y (which is default
and basically reverted) in CIs. Clang, OTOH, has currently no issues with that.

Andy Shevchenko (4):
  Input: ALPS - increase size of phys2 and phys3
  Input: atkbd - increase size of phys
  Input: lifebook - increase size of phys
  Input: psmouse - increase size of phys

 drivers/input/keyboard/atkbd.c | 2 +-
 drivers/input/mouse/alps.h     | 4 ++--
 drivers/input/mouse/lifebook.c | 2 +-
 drivers/input/mouse/psmouse.h  | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

-- 
2.47.2

[PATCH v1 1/4] Input: ALPS - increase size of phys2 and phys3

[Andy Shevchenko]

When creating a physical device name in the driver the snprintf() takes
an up to 32 characters argument along with the additional 8 characters
and tries to pack this into 32 bytes array. GCC complains about that
when build with `make W=1`:

drivers/input/mouse/alps.c:1411:9: note: ‘snprintf’ output between 8 and 39 bytes into a destination of size 32
 1411 |         snprintf(priv->phys3, sizeof(priv->phys3), "%s/%s",
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1412 |                  psmouse->ps2dev.serio->phys,
      |                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1413 |                  (priv->dev2 ? "input2" : "input1"));
      |                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

drivers/input/mouse/alps.c:3106:17: note: ‘snprintf’ output between 8 and 39 bytes into a destination of size 32
 3106 |                 snprintf(priv->phys2, sizeof(priv->phys2), "%s/input1",
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 3107 |                          psmouse->ps2dev.serio->phys);
      |                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Increase the size to cover all possible cases.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/input/mouse/alps.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h
index 0a1048cf23f6..9c8f69694f60 100644
--- a/drivers/input/mouse/alps.h
+++ b/drivers/input/mouse/alps.h
@@ -287,8 +287,8 @@ struct alps_data {
 	struct psmouse *psmouse;
 	struct input_dev *dev2;
 	struct input_dev *dev3;
-	char phys2[32];
-	char phys3[32];
+	char phys2[40];
+	char phys3[40];
 	struct delayed_work dev3_register_work;
 
 	/* these are autodetected when the device is identified */
-- 
2.47.2

[PATCH v1 2/4] Input: atkbd - increase size of phys

[Andy Shevchenko]

When creating a physical device name in the driver the snprintf() takes
an up to 32 characters argument along with the additional 8 characters
and tries to pack this into 32 bytes array. GCC complains about that
when build with `make W=1`:

drivers/input/keyboard/atkbd.c:1194:9: note: ‘snprintf’ output between 8 and 39 bytes into a destination of size 32
 1194 |         snprintf(atkbd->phys, sizeof(atkbd->phys),
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1195 |                  "%s/input0", atkbd->ps2dev.serio->phys);
      |                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Increase the size to cover all possible cases.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/input/keyboard/atkbd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index adf0f311996c..3bf76dc768be 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -206,7 +206,7 @@ struct atkbd {
 
 	/* Written only during init */
 	char name[64];
-	char phys[32];
+	char phys[40];
 
 	unsigned short id;
 	unsigned short keycode[ATKBD_KEYMAP_SIZE];
-- 
2.47.2

[PATCH v1 3/4] Input: lifebook - increase size of phys

[Andy Shevchenko]

When creating a physical device name in the driver the snprintf() takes
an up to 32 characters argument along with the additional 8 characters
and tries to pack this into 32 bytes array. GCC complains about that
when build with `make W=1`:

drivers/input/mouse/lifebook.c:282:9: note: ‘snprintf’ output between 8 and 39 bytes into a destination of size 32
 282 |         snprintf(priv->phys, sizeof(priv->phys),
     |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 283 |                  "%s/input1", psmouse->ps2dev.serio->phys);
     |                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Increase the size to cover all possible cases.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/input/mouse/lifebook.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/mouse/lifebook.c b/drivers/input/mouse/lifebook.c
index 7147dacc404f..628a1b36a5da 100644
--- a/drivers/input/mouse/lifebook.c
+++ b/drivers/input/mouse/lifebook.c
@@ -21,7 +21,7 @@
 
 struct lifebook_data {
 	struct input_dev *dev2;		/* Relative device */
-	char phys[32];
+	char phys[40];
 };
 
 static bool lifebook_present;
-- 
2.47.2

[PATCH v1 4/4] Input: psmouse - increase size of phys

[Andy Shevchenko]

When creating a physical device name in the driver the snprintf() takes
an up to 32 characters argument along with the additional 8 characters
and tries to pack this into 32 bytes array. GCC complains about that
when build with `make W=1`:

drivers/input/mouse/psmouse-base.c:1603:9: note: ‘snprintf’ output between 8 and 39 bytes into a destination of size 32
 1603 |         snprintf(psmouse->phys, sizeof(psmouse->phys), "%s/input0", serio->phys);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Increase the size to cover all possible cases.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/input/mouse/psmouse.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/mouse/psmouse.h b/drivers/input/mouse/psmouse.h
index 4d8acfe0d82a..8422ee8243bb 100644
--- a/drivers/input/mouse/psmouse.h
+++ b/drivers/input/mouse/psmouse.h
@@ -107,7 +107,7 @@ struct psmouse {
 	unsigned long num_resyncs;
 	enum psmouse_state state;
 	char devname[64];
-	char phys[32];
+	char phys[40];
 
 	unsigned int rate;
 	unsigned int resolution;
-- 
2.47.2

Re: [PATCH v1 0/4] Input: Increase size of phys in the drivers

[Andy Shevchenko]

On Fri, Feb 28, 2025 at 02:07:43PM +0200, Andy Shevchenko wrote:
> The drivers are using local member of 32 bytes to hold up to 40 (one-byte)
> characters. GCC complains on that. This series fixes the issue in the affected
> input drivers. Note, this is currently the biggest part of the warnings that
> are being treated as errors with the default configurations on x86. With this
> being applied we become quite close to enable CONFIG_WERROR=y (which is default
> and basically reverted) in CIs. Clang, OTOH, has currently no issues with that.

If acceptable, it would be good to have them as fixes for v6.14 cycle.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/4] Input: Increase size of phys in the drivers

[Andy Shevchenko]

On Fri, Feb 28, 2025 at 02:07:43PM +0200, Andy Shevchenko wrote:
> The drivers are using local member of 32 bytes to hold up to 40 (one-byte)
> characters. GCC complains on that. This series fixes the issue in the affected
> input drivers. Note, this is currently the biggest part of the warnings that
> are being treated as errors with the default configurations on x86. With this
> being applied we become quite close to enable CONFIG_WERROR=y (which is default
> and basically reverted) in CIs. Clang, OTOH, has currently no issues with that.

Would be nice to have a comment on this rather sooner as this impacts
the compilation by `make W=1` with WERROR=y (which is default).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/4] Input: Increase size of phys in the drivers

[Dmitry Torokhov]

Hi Andy,

On Tue, Mar 04, 2025 at 02:14:12PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 28, 2025 at 02:07:43PM +0200, Andy Shevchenko wrote:
> > The drivers are using local member of 32 bytes to hold up to 40 (one-byte)
> > characters. GCC complains on that. This series fixes the issue in the affected
> > input drivers. Note, this is currently the biggest part of the warnings that
> > are being treated as errors with the default configurations on x86. With this
> > being applied we become quite close to enable CONFIG_WERROR=y (which is default
> > and basically reverted) in CIs. Clang, OTOH, has currently no issues with that.
> 
> Would be nice to have a comment on this rather sooner as this impacts
> the compilation by `make W=1` with WERROR=y (which is default).

I do not like the change. There are no bugs, only GCC being paranoid.
Are there any other ways to shut it up? In [1] Jeff says that switching
to scnprintf() shuts GCC up...

[1] https://lore.kernel.org/r/Z3rIvp0hzS+yzvJA@nixie71

Thanks.

-- 
Dmitry

Re: [PATCH v1 0/4] Input: Increase size of phys in the drivers

[Andy Shevchenko]

On Tue, Mar 04, 2025 at 11:18:52PM -0800, Dmitry Torokhov wrote:
> On Tue, Mar 04, 2025 at 02:14:12PM +0200, Andy Shevchenko wrote:
> > On Fri, Feb 28, 2025 at 02:07:43PM +0200, Andy Shevchenko wrote:
> > > The drivers are using local member of 32 bytes to hold up to 40 (one-byte)
> > > characters. GCC complains on that. This series fixes the issue in the affected
> > > input drivers. Note, this is currently the biggest part of the warnings that
> > > are being treated as errors with the default configurations on x86. With this
> > > being applied we become quite close to enable CONFIG_WERROR=y (which is default
> > > and basically reverted) in CIs. Clang, OTOH, has currently no issues with that.
> > 
> > Would be nice to have a comment on this rather sooner as this impacts
> > the compilation by `make W=1` with WERROR=y (which is default).
> 
> I do not like the change.

Independently on your opinion in this case GCC is correct.
We are trying to squeeze up to 40 bytes into 32-byte storage.
I.o.w. GCC can't prove that and reader of the code can't prove
that either.

> There are no bugs, only GCC being paranoid.

I'm not so sure. But probably it works because the user space is parsing full
"inputX" string in the names

> Are there any other ways to shut it up? In [1] Jeff says that switching
> to scnprintf() shuts GCC up...

I do not like this, because it's just a hiding the problem and not solving it.
At some point GCC may start issuing warning on those cases as well when it
realizes the above. If you like that solution, please fix in that way. We have
4 drivers break the compilation currently.

> [1] https://lore.kernel.org/r/Z3rIvp0hzS+yzvJA@nixie71

So, consider this series as a bug report that prevents compilation.
I would expect somebody to fix this rather sooner than later.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/4] Input: Increase size of phys in the drivers

[Andy Shevchenko]

On Wed, Mar 05, 2025 at 12:06:25PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 04, 2025 at 11:18:52PM -0800, Dmitry Torokhov wrote:
> > On Tue, Mar 04, 2025 at 02:14:12PM +0200, Andy Shevchenko wrote:
> > > On Fri, Feb 28, 2025 at 02:07:43PM +0200, Andy Shevchenko wrote:
> > > > The drivers are using local member of 32 bytes to hold up to 40 (one-byte)
> > > > characters. GCC complains on that. This series fixes the issue in the affected
> > > > input drivers. Note, this is currently the biggest part of the warnings that
> > > > are being treated as errors with the default configurations on x86. With this
> > > > being applied we become quite close to enable CONFIG_WERROR=y (which is default
> > > > and basically reverted) in CIs. Clang, OTOH, has currently no issues with that.
> > > 
> > > Would be nice to have a comment on this rather sooner as this impacts
> > > the compilation by `make W=1` with WERROR=y (which is default).
> > 
> > I do not like the change.
> 
> Independently on your opinion in this case GCC is correct.
> We are trying to squeeze up to 40 bytes into 32-byte storage.
> I.o.w. GCC can't prove that and reader of the code can't prove
> that either.
> 
> > There are no bugs, only GCC being paranoid.
> 
> I'm not so sure. But probably it works because the user space is parsing full
> "inputX" string in the names
> 
> > Are there any other ways to shut it up? In [1] Jeff says that switching
> > to scnprintf() shuts GCC up...
> 
> I do not like this, because it's just a hiding the problem and not solving it.
> At some point GCC may start issuing warning on those cases as well when it
> realizes the above. If you like that solution, please fix in that way. We have
> 4 drivers break the compilation currently.

Actually looking closer, the better fix (and which I'm not against) is to check
for returned value of snprintf() and act accordingly. What do you think?

> > [1] https://lore.kernel.org/r/Z3rIvp0hzS+yzvJA@nixie71
> 
> So, consider this series as a bug report that prevents compilation.
> I would expect somebody to fix this rather sooner than later.

-- 
With Best Regards,
Andy Shevchenko