* [PATCH] Input: wacom_w8001: Check for string overflow from strscpy calls
@ 2024-05-10 23:43 joshua
2024-05-20 18:00 ` Ping Cheng
0 siblings, 1 reply; 6+ messages in thread
From: joshua @ 2024-05-10 23:43 UTC (permalink / raw)
To: linux-input, Jiri Kosina, Benjamin Tissoires
Cc: Ping Cheng, Aaron Armstrong Skomra, Jason Gerecke,
Joshua Dickens, Joshua Dickens
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Input: wacom_w8001: Check for string overflow from strscpy calls
2024-05-10 23:43 [PATCH] Input: wacom_w8001: Check for string overflow from strscpy calls joshua
@ 2024-05-20 18:00 ` Ping Cheng
2024-05-20 23:28 ` Dmitry Torokhov
0 siblings, 1 reply; 6+ messages in thread
From: Ping Cheng @ 2024-05-20 18:00 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, Aaron Armstrong Skomra, Jason Gerecke,
Joshua Dickens, Joshua Dickens
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Input: wacom_w8001: Check for string overflow from strscpy calls
2024-05-20 18:00 ` Ping Cheng
@ 2024-05-20 23:28 ` Dmitry Torokhov
2024-05-21 3:55 ` Ping Cheng
0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Torokhov @ 2024-05-20 23:28 UTC (permalink / raw)
To: Ping Cheng
Cc: linux-input, Aaron Armstrong Skomra, Jason Gerecke,
Joshua Dickens, Joshua Dickens
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Input: wacom_w8001: Check for string overflow from strscpy calls
2024-05-20 23:28 ` Dmitry Torokhov
@ 2024-05-21 3:55 ` Ping Cheng
2024-05-22 17:23 ` Dmitry Torokhov
0 siblings, 1 reply; 6+ messages in thread
From: Ping Cheng @ 2024-05-21 3:55 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, Aaron Armstrong Skomra, Jason Gerecke,
Joshua Dickens, Joshua Dickens
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Input: wacom_w8001: Check for string overflow from strscpy calls
2024-05-21 3:55 ` Ping Cheng
@ 2024-05-22 17:23 ` Dmitry Torokhov
2024-05-23 16:51 ` Jason Gerecke
0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Torokhov @ 2024-05-22 17:23 UTC (permalink / raw)
To: Ping Cheng
Cc: linux-input, Aaron Armstrong Skomra, Jason Gerecke,
Joshua Dickens, Joshua Dickens
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Input: wacom_w8001: Check for string overflow from strscpy calls
2024-05-22 17:23 ` Dmitry Torokhov
@ 2024-05-23 16:51 ` Jason Gerecke
0 siblings, 0 replies; 6+ messages in thread
From: Jason Gerecke @ 2024-05-23 16:51 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Ping Cheng, linux-input, Aaron Armstrong Skomra, Joshua Dickens,
Joshua Dickens
[-- 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-05-23 16:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-05-10 23:43 [PATCH] Input: wacom_w8001: Check for string overflow from strscpy calls joshua
2024-05-20 18:00 ` Ping Cheng
2024-05-20 23:28 ` Dmitry Torokhov
2024-05-21 3:55 ` Ping Cheng
2024-05-22 17:23 ` Dmitry Torokhov
2024-05-23 16:51 ` Jason Gerecke
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).