diff mbox series

Input: wacom_w8001: Check for string overflow from strscpy calls

Message ID 20240510234332.139038-1-Joshua@Joshua-Dickens.com (mailing list archive)
State New
Headers show
Series Input: wacom_w8001: Check for string overflow from strscpy calls | expand

Commit Message

Joshua Dickens May 10, 2024, 11:43 p.m. UTC 
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(-)

Comments

Ping Cheng May 20, 2024, 6 p.m. UTC | #1 
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
>
Dmitry Torokhov May 20, 2024, 11:28 p.m. UTC | #2 
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.
Ping Cheng May 21, 2024, 3:55 a.m. UTC | #3 
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
Dmitry Torokhov May 22, 2024, 5:23 p.m. UTC | #4 
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.
Gerecke, Jason May 23, 2024, 4:51 p.m. UTC | #5 
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
Dmitry Torokhov May 30, 2024, 11:48 p.m. UTC | #6 
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.
diff mbox series

Patch

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;