Input: wacom_w8001: Check for string overflow from strscpy calls

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(-)

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
>

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.

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

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.

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.