[PATCH] Input: wacom_w8001: Check for string overflow from strscpy calls

[PATCH] Input: wacom_w8001: Check for string overflow from strscpy calls

[joshua]

From: Joshua Dickens <Joshua@Joshua-Dickens.com>

The strscpy function is able to return an error code when a copy would
overflow the size of the destination. The copy is stopped and the buffer
terminated before overflow actually occurs so it is safe to continue
execution, but we should still produce a warning should this occur.

Signed-off-by: Joshua Dickens <joshua.dickens@wacom.com>
---
 drivers/input/touchscreen/wacom_w8001.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
index 928c5ee3ac36..b8acf196a09a 100644
--- a/drivers/input/touchscreen/wacom_w8001.c
+++ b/drivers/input/touchscreen/wacom_w8001.c
@@ -625,7 +625,10 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
 	/* For backwards-compatibility we compose the basename based on
 	 * capabilities and then just append the tool type
 	 */
-	strscpy(basename, "Wacom Serial", sizeof(basename));
+	if (strscpy(basename, "Wacom Serial", sizeof(basename)) < 0) {
+		dev_warn(&w8001->pen_dev->dev,
+			"String overflow while assembling basename");
+	}
 
 	err_pen = w8001_setup_pen(w8001, basename, sizeof(basename));
 	err_touch = w8001_setup_touch(w8001, basename, sizeof(basename));
@@ -635,7 +638,11 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
 	}
 
 	if (!err_pen) {
-		strscpy(w8001->pen_name, basename, sizeof(w8001->pen_name));
+		if (strscpy(w8001->pen_name, basename,
+			sizeof(w8001->pen_name)) < 0) {
+			dev_warn(&w8001->pen_dev->dev,
+				"String overflow while assembling pen_name");
+		}
 		strlcat(w8001->pen_name, " Pen", sizeof(w8001->pen_name));
 		input_dev_pen->name = w8001->pen_name;
 
@@ -651,7 +658,11 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
 	}
 
 	if (!err_touch) {
-		strscpy(w8001->touch_name, basename, sizeof(w8001->touch_name));
+		if (strscpy(w8001->touch_name, basename,
+			sizeof(w8001->touch_name)) < 0) {
+			dev_warn(&w8001->touch_dev->dev,
+				"String overflow while assembling touch_name");
+		}
 		strlcat(w8001->touch_name, " Finger",
 			sizeof(w8001->touch_name));
 		input_dev_touch->name = w8001->touch_name;
-- 
2.45.0

Re: [PATCH] Input: wacom_w8001: Check for string overflow from strscpy calls

[Ping Cheng]

Hi Dmitry,

This fix is the same as [1]. Without checking the return value,
Wolfram's patch [2] fails our downstream build script. I'm adding my
r-b, if it makes any difference ;).

Reviewed-by: Ping Cheng <ping.cheng@wacom.com>

Thank you,
Ping

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/hid/wacom_sys.c?id=d9eef346b601afb0bd74b49e0db06f6a5cebd030
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/input/touchscreen/wacom_w8001.c?id=a9f08ad7adb3d2f90e11efbb40a1246ef95b0c04


On Fri, May 10, 2024 at 4:43 PM <joshua@joshua-dickens.com> wrote:
>
> From: Joshua Dickens <Joshua@Joshua-Dickens.com>
>
> The strscpy function is able to return an error code when a copy would
> overflow the size of the destination. The copy is stopped and the buffer
> terminated before overflow actually occurs so it is safe to continue
> execution, but we should still produce a warning should this occur.
>
> Signed-off-by: Joshua Dickens <joshua.dickens@wacom.com>
> ---
>  drivers/input/touchscreen/wacom_w8001.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
> index 928c5ee3ac36..b8acf196a09a 100644
> --- a/drivers/input/touchscreen/wacom_w8001.c
> +++ b/drivers/input/touchscreen/wacom_w8001.c
> @@ -625,7 +625,10 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
>         /* For backwards-compatibility we compose the basename based on
>          * capabilities and then just append the tool type
>          */
> -       strscpy(basename, "Wacom Serial", sizeof(basename));
> +       if (strscpy(basename, "Wacom Serial", sizeof(basename)) < 0) {
> +               dev_warn(&w8001->pen_dev->dev,
> +                       "String overflow while assembling basename");
> +       }
>
>         err_pen = w8001_setup_pen(w8001, basename, sizeof(basename));
>         err_touch = w8001_setup_touch(w8001, basename, sizeof(basename));
> @@ -635,7 +638,11 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
>         }
>
>         if (!err_pen) {
> -               strscpy(w8001->pen_name, basename, sizeof(w8001->pen_name));
> +               if (strscpy(w8001->pen_name, basename,
> +                       sizeof(w8001->pen_name)) < 0) {
> +                       dev_warn(&w8001->pen_dev->dev,
> +                               "String overflow while assembling pen_name");
> +               }
>                 strlcat(w8001->pen_name, " Pen", sizeof(w8001->pen_name));
>                 input_dev_pen->name = w8001->pen_name;
>
> @@ -651,7 +658,11 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
>         }
>
>         if (!err_touch) {
> -               strscpy(w8001->touch_name, basename, sizeof(w8001->touch_name));
> +               if (strscpy(w8001->touch_name, basename,
> +                       sizeof(w8001->touch_name)) < 0) {
> +                       dev_warn(&w8001->touch_dev->dev,
> +                               "String overflow while assembling touch_name");
> +               }
>                 strlcat(w8001->touch_name, " Finger",
>                         sizeof(w8001->touch_name));
>                 input_dev_touch->name = w8001->touch_name;
> --
> 2.45.0
>

Re: [PATCH] Input: wacom_w8001: Check for string overflow from strscpy calls

[Dmitry Torokhov]

Hi Ping,

On Mon, May 20, 2024 at 11:00:26AM -0700, Ping Cheng wrote:
> Hi Dmitry,
> 
> This fix is the same as [1]. Without checking the return value,
> Wolfram's patch [2] fails our downstream build script. I'm adding my
> r-b, if it makes any difference ;).

Could you please tell me how exactly it makes your build script to fail?

My concern is that the warnings are not actionable and therefore pretty
much worthless.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: wacom_w8001: Check for string overflow from strscpy calls

[Ping Cheng]

On Mon, May 20, 2024 at 4:28 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

Hi Dmitry,

> > This fix is the same as [1]. Without checking the return value,
> > Wolfram's patch [2] fails our downstream build script. I'm adding my
> > r-b, if it makes any difference ;).
>
> Could you please tell me how exactly it makes your build script to fail?

We got an "unused-result warning". Jason made a temporary workaround
at https://github.com/linuxwacom/input-wacom/commit/e83a9bb3e48d2d1b52ec709e60f73b9960d568e5.

> My concern is that the warnings are not actionable and therefore pretty
> much worthless.

The return value tells us which strscpy call(s) didn't have enough space.

Cheers,
Ping

Re: [PATCH] Input: wacom_w8001: Check for string overflow from strscpy calls

[Dmitry Torokhov]

On Mon, May 20, 2024 at 08:55:30PM -0700, Ping Cheng wrote:
> On Mon, May 20, 2024 at 4:28 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> 
> Hi Dmitry,
> 
> > > This fix is the same as [1]. Without checking the return value,
> > > Wolfram's patch [2] fails our downstream build script. I'm adding my
> > > r-b, if it makes any difference ;).
> >
> > Could you please tell me how exactly it makes your build script to fail?
> 
> We got an "unused-result warning". Jason made a temporary workaround
> at https://github.com/linuxwacom/input-wacom/commit/e83a9bb3e48d2d1b52ec709e60f73b9960d568e5.

I do not think strscpy is declared as __must_check. Do you have a repro
for the vanilla kernel build?

> 
> > My concern is that the warnings are not actionable and therefore pretty
> > much worthless.
> 
> The return value tells us which strscpy call(s) didn't have enough space.

Right, and what can be done about it? The driver does not control what
tty is being used for the serial port (and so can't control the prefix
of the 'phys'), we do not extend 'phys' (because it is used very
rarely). The only option is to truncate (which we already do).

So the warnings are not actionable.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: wacom_w8001: Check for string overflow from strscpy calls

[Jason Gerecke]

[-- Attachment #1: Type: text/plain, Size: 3021 bytes --]

On Wed, May 22, 2024 at 10:23 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Mon, May 20, 2024 at 08:55:30PM -0700, Ping Cheng wrote:
> > On Mon, May 20, 2024 at 4:28 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> >
> > Hi Dmitry,
> >
> > > > This fix is the same as [1]. Without checking the return value,
> > > > Wolfram's patch [2] fails our downstream build script. I'm adding my
> > > > r-b, if it makes any difference ;).
> > >
> > > Could you please tell me how exactly it makes your build script to fail?
> >
> > We got an "unused-result warning". Jason made a temporary workaround
> > at https://github.com/linuxwacom/input-wacom/commit/e83a9bb3e48d2d1b52ec709e60f73b9960d568e5.
>
> I do not think strscpy is declared as __must_check. Do you have a repro
> for the vanilla kernel build?
>

There is no build error with the upstream code. The build error occurs
when compiling an updated version of wacom_w8001.c on older (pre-4.16)
kernels. These older kernels did declare strscpy as __must_check.

We maintain backports of the upstream sources for users who are stuck
running old kernels. To ease the maintenance burden, we desire to keep
the upstream/downstream diff as small as possible. We understand that
not every upstream change is compatible with older kernels and will
introduce changes where necessary. In this particular situation we
believed that addressing the warning upstream made more sense than
maintaining a diff. It isn't wrong for upstream to verify return
values are sane, even if we admittedly do expect it to be a bit
unnecessary here.

> >
> > > My concern is that the warnings are not actionable and therefore pretty
> > > much worthless.
> >

IMO, the vast majority of the worth of this patch is a minimized
upstream / downstream diff -- not the warning itself. I know that this
means that such a patch is "not your problem", but it doesn't mean
that it is "nobody's" problem. If you don't like the idea of
introducing non-actionable warnings, would you be open to a small
cleanup patch instead (see attached as an example)? There's no
particularly good reason to split string generation across calls to
strscpy and strlcat when a single call to snprintf would do, and
snprintf is not __must_check on any of the kernels we backport to.

Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one  /
(That is to say, eight) to the two,     /
But you can’t take seven from three,    /
So you look at the sixty-fours....

> > The return value tells us which strscpy call(s) didn't have enough space.
>
> Right, and what can be done about it? The driver does not control what
> tty is being used for the serial port (and so can't control the prefix
> of the 'phys'), we do not extend 'phys' (because it is used very
> rarely). The only option is to truncate (which we already do).
>
> So the warnings are not actionable.
>
> Thanks.
>
> --
> Dmitry

[-- Attachment #2: 0001-input-wacom_w8001-Simplify-device-name-generation.patch --]
[-- Type: text/x-patch, Size: 2306 bytes --]

From 3b13656819a2c5dbf2a1c1f8fb407595eb3e5d94 Mon Sep 17 00:00:00 2001
From: Jason Gerecke <jason.gerecke@wacom.com>
Date: Thu, 23 May 2024 08:58:56 -0700
Subject: [PATCH] input: wacom_w8001: Simplify device name generation

Replace pairs of strscpy/strlcat calls with snprintf.

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
 drivers/input/touchscreen/wacom_w8001.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
index 928c5ee3ac36c..e3767db8a664b 100644
--- a/drivers/input/touchscreen/wacom_w8001.c
+++ b/drivers/input/touchscreen/wacom_w8001.c
@@ -595,7 +595,7 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
 	struct w8001 *w8001;
 	struct input_dev *input_dev_pen;
 	struct input_dev *input_dev_touch;
-	char basename[64];
+	char basename[64] = "Wacom Serial";
 	int err, err_pen, err_touch;
 
 	w8001 = kzalloc(sizeof(struct w8001), GFP_KERNEL);
@@ -625,8 +625,6 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
 	/* For backwards-compatibility we compose the basename based on
 	 * capabilities and then just append the tool type
 	 */
-	strscpy(basename, "Wacom Serial", sizeof(basename));
-
 	err_pen = w8001_setup_pen(w8001, basename, sizeof(basename));
 	err_touch = w8001_setup_touch(w8001, basename, sizeof(basename));
 	if (err_pen && err_touch) {
@@ -635,8 +633,7 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
 	}
 
 	if (!err_pen) {
-		strscpy(w8001->pen_name, basename, sizeof(w8001->pen_name));
-		strlcat(w8001->pen_name, " Pen", sizeof(w8001->pen_name));
+		snprintf(w8001->pen_name, sizeof(w8001->pen_name), "%s Pen", basename);
 		input_dev_pen->name = w8001->pen_name;
 
 		w8001_set_devdata(input_dev_pen, w8001, serio);
@@ -651,9 +648,7 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
 	}
 
 	if (!err_touch) {
-		strscpy(w8001->touch_name, basename, sizeof(w8001->touch_name));
-		strlcat(w8001->touch_name, " Finger",
-			sizeof(w8001->touch_name));
+		snprintf(w8001->pen_name, sizeof(w8001->pen_name), "%s Finger", basename);
 		input_dev_touch->name = w8001->touch_name;
 
 		w8001_set_devdata(input_dev_touch, w8001, serio);
-- 
2.45.1

Re: [PATCH] Input: wacom_w8001: Check for string overflow from strscpy calls

[Dmitry Torokhov]

On Thu, May 23, 2024 at 09:51:42AM -0700, Jason Gerecke wrote:
> [...] If you don't like the idea of introducing non-actionable
> warnings, would you be open to a small cleanup patch instead (see
> attached as an example)? There's no particularly good reason to split
> string generation across calls to strscpy and strlcat when a single
> call to snprintf would do, and snprintf is not __must_check on any of
> the kernels we backport to.

Yes, I actually like it much better, applied.

Thanks.

-- 
Dmitry

[PATCH] Input: wacom_w8001: Correct device name generation

[Gerecke, Jason]

From: Jason Gerecke <jason.gerecke@wacom.com>

Fixes: 6c7cc1a29d1e ("Input: wacom_w8001 - simplify device name generation")
Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
 drivers/input/touchscreen/wacom_w8001.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
index e3767db8a664b..ed9cfdd616cac 100644
--- a/drivers/input/touchscreen/wacom_w8001.c
+++ b/drivers/input/touchscreen/wacom_w8001.c
@@ -648,7 +648,7 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
 	}
 
 	if (!err_touch) {
-		snprintf(w8001->pen_name, sizeof(w8001->pen_name), "%s Finger", basename);
+		snprintf(w8001->touch_name, sizeof(w8001->touch_name), "%s Finger", basename);
 		input_dev_touch->name = w8001->touch_name;
 
 		w8001_set_devdata(input_dev_touch, w8001, serio);
-- 
2.45.2

Re: [PATCH] Input: wacom_w8001: Correct device name generation

[Dmitry Torokhov]

On Wed, Jun 05, 2024 at 09:46:56AM -0700, Gerecke, Jason wrote:
> From: Jason Gerecke <jason.gerecke@wacom.com>
> 
> Fixes: 6c7cc1a29d1e ("Input: wacom_w8001 - simplify device name generation")
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>

Applied, thank you.

-- 
Dmitry