[PATCH] HID: wacom: Check for string overflow from strscpy calls

[PATCH] HID: wacom: Check for string overflow from strscpy calls

[Jason Gerecke]

From: Jason Gerecke <killertofu@gmail.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: Jason Gerecke <jason.gerecke@wacom.com>
Reviewed-by: Ping Cheng <ping.cheng@wacom.com>
---
 drivers/hid/wacom_sys.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 8214896adadad..7192970d199a0 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -2224,7 +2224,9 @@ static void wacom_update_name(struct wacom *wacom, const char *suffix)
 		} else if (strstr(product_name, "Wacom") ||
 			   strstr(product_name, "wacom") ||
 			   strstr(product_name, "WACOM")) {
-			strscpy(name, product_name, sizeof(name));
+			if (strscpy(name, product_name, sizeof(name)) < 0) {
+				hid_warn(wacom->hdev, "String overflow while assembling device name");
+			}
 		} else {
 			snprintf(name, sizeof(name), "Wacom %s", product_name);
 		}
@@ -2242,7 +2244,9 @@ static void wacom_update_name(struct wacom *wacom, const char *suffix)
 		if (name[strlen(name)-1] == ' ')
 			name[strlen(name)-1] = '\0';
 	} else {
-		strscpy(name, features->name, sizeof(name));
+		if (strscpy(name, features->name, sizeof(name)) < 0) {
+			hid_warn(wacom->hdev, "String overflow while assembling device name");
+		}
 	}
 
 	snprintf(wacom_wac->name, sizeof(wacom_wac->name), "%s%s",
@@ -2500,8 +2504,10 @@ static void wacom_wireless_work(struct work_struct *work)
 				goto fail;
 		}
 
-		strscpy(wacom_wac->name, wacom_wac1->name,
-			sizeof(wacom_wac->name));
+		if (strscpy(wacom_wac->name, wacom_wac1->name,
+			sizeof(wacom_wac->name)) < 0) {
+			hid_warn(wacom->hdev, "String overflow while assembling device name");
+		}
 	}
 
 	return;
-- 
2.40.0

Re: [PATCH] HID: wacom: Check for string overflow from strscpy calls

[Jason Gerecke]

On Fri, Apr 14, 2023 at 11:22 AM Jason Gerecke <killertofu@gmail.com> wrote:
>
> From: Jason Gerecke <killertofu@gmail.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: Jason Gerecke <jason.gerecke@wacom.com>
> Reviewed-by: Ping Cheng <ping.cheng@wacom.com>
> ---
>  drivers/hid/wacom_sys.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 8214896adadad..7192970d199a0 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -2224,7 +2224,9 @@ static void wacom_update_name(struct wacom *wacom, const char *suffix)
>                 } else if (strstr(product_name, "Wacom") ||
>                            strstr(product_name, "wacom") ||
>                            strstr(product_name, "WACOM")) {
> -                       strscpy(name, product_name, sizeof(name));
> +                       if (strscpy(name, product_name, sizeof(name)) < 0) {
> +                               hid_warn(wacom->hdev, "String overflow while assembling device name");
> +                       }
>                 } else {
>                         snprintf(name, sizeof(name), "Wacom %s", product_name);
>                 }
> @@ -2242,7 +2244,9 @@ static void wacom_update_name(struct wacom *wacom, const char *suffix)
>                 if (name[strlen(name)-1] == ' ')
>                         name[strlen(name)-1] = '\0';
>         } else {
> -               strscpy(name, features->name, sizeof(name));
> +               if (strscpy(name, features->name, sizeof(name)) < 0) {
> +                       hid_warn(wacom->hdev, "String overflow while assembling device name");
> +               }
>         }
>
>         snprintf(wacom_wac->name, sizeof(wacom_wac->name), "%s%s",
> @@ -2500,8 +2504,10 @@ static void wacom_wireless_work(struct work_struct *work)
>                                 goto fail;
>                 }
>
> -               strscpy(wacom_wac->name, wacom_wac1->name,
> -                       sizeof(wacom_wac->name));
> +               if (strscpy(wacom_wac->name, wacom_wac1->name,
> +                       sizeof(wacom_wac->name)) < 0) {
> +                       hid_warn(wacom->hdev, "String overflow while assembling device name");
> +               }
>         }
>
>         return;
> --
> 2.40.0
>

Poking this thread again in case it got lost in the inbox...

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....

Re: [PATCH] HID: wacom: Check for string overflow from strscpy calls

[Peter Hutterer]

On Fri, Apr 14, 2023 at 11:22:10AM -0700, Jason Gerecke wrote:
> From: Jason Gerecke <killertofu@gmail.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: Jason Gerecke <jason.gerecke@wacom.com>
> Reviewed-by: Ping Cheng <ping.cheng@wacom.com>

Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>

Cheers,
  Peter

> ---
>  drivers/hid/wacom_sys.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 8214896adadad..7192970d199a0 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -2224,7 +2224,9 @@ static void wacom_update_name(struct wacom *wacom, const char *suffix)
>  		} else if (strstr(product_name, "Wacom") ||
>  			   strstr(product_name, "wacom") ||
>  			   strstr(product_name, "WACOM")) {
> -			strscpy(name, product_name, sizeof(name));
> +			if (strscpy(name, product_name, sizeof(name)) < 0) {
> +				hid_warn(wacom->hdev, "String overflow while assembling device name");
> +			}
>  		} else {
>  			snprintf(name, sizeof(name), "Wacom %s", product_name);
>  		}
> @@ -2242,7 +2244,9 @@ static void wacom_update_name(struct wacom *wacom, const char *suffix)
>  		if (name[strlen(name)-1] == ' ')
>  			name[strlen(name)-1] = '\0';
>  	} else {
> -		strscpy(name, features->name, sizeof(name));
> +		if (strscpy(name, features->name, sizeof(name)) < 0) {
> +			hid_warn(wacom->hdev, "String overflow while assembling device name");
> +		}
>  	}
>  
>  	snprintf(wacom_wac->name, sizeof(wacom_wac->name), "%s%s",
> @@ -2500,8 +2504,10 @@ static void wacom_wireless_work(struct work_struct *work)
>  				goto fail;
>  		}
>  
> -		strscpy(wacom_wac->name, wacom_wac1->name,
> -			sizeof(wacom_wac->name));
> +		if (strscpy(wacom_wac->name, wacom_wac1->name,
> +			sizeof(wacom_wac->name)) < 0) {
> +			hid_warn(wacom->hdev, "String overflow while assembling device name");
> +		}
>  	}
>  
>  	return;
> -- 
> 2.40.0
> 

Re: [PATCH] HID: wacom: Check for string overflow from strscpy calls

[Jason Gerecke]

On Wed, May 3, 2023 at 9:34 PM Peter Hutterer <peter.hutterer@who-t.net> wrote:
>
> On Fri, Apr 14, 2023 at 11:22:10AM -0700, Jason Gerecke wrote:
> > From: Jason Gerecke <killertofu@gmail.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: Jason Gerecke <jason.gerecke@wacom.com>
> > Reviewed-by: Ping Cheng <ping.cheng@wacom.com>
>
> Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
>
> Cheers,
>   Peter
>

Sending another request for follow-up.

Jason

> > ---
> >  drivers/hid/wacom_sys.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> > index 8214896adadad..7192970d199a0 100644
> > --- a/drivers/hid/wacom_sys.c
> > +++ b/drivers/hid/wacom_sys.c
> > @@ -2224,7 +2224,9 @@ static void wacom_update_name(struct wacom *wacom, const char *suffix)
> >               } else if (strstr(product_name, "Wacom") ||
> >                          strstr(product_name, "wacom") ||
> >                          strstr(product_name, "WACOM")) {
> > -                     strscpy(name, product_name, sizeof(name));
> > +                     if (strscpy(name, product_name, sizeof(name)) < 0) {
> > +                             hid_warn(wacom->hdev, "String overflow while assembling device name");
> > +                     }
> >               } else {
> >                       snprintf(name, sizeof(name), "Wacom %s", product_name);
> >               }
> > @@ -2242,7 +2244,9 @@ static void wacom_update_name(struct wacom *wacom, const char *suffix)
> >               if (name[strlen(name)-1] == ' ')
> >                       name[strlen(name)-1] = '\0';
> >       } else {
> > -             strscpy(name, features->name, sizeof(name));
> > +             if (strscpy(name, features->name, sizeof(name)) < 0) {
> > +                     hid_warn(wacom->hdev, "String overflow while assembling device name");
> > +             }
> >       }
> >
> >       snprintf(wacom_wac->name, sizeof(wacom_wac->name), "%s%s",
> > @@ -2500,8 +2504,10 @@ static void wacom_wireless_work(struct work_struct *work)
> >                               goto fail;
> >               }
> >
> > -             strscpy(wacom_wac->name, wacom_wac1->name,
> > -                     sizeof(wacom_wac->name));
> > +             if (strscpy(wacom_wac->name, wacom_wac1->name,
> > +                     sizeof(wacom_wac->name)) < 0) {
> > +                     hid_warn(wacom->hdev, "String overflow while assembling device name");
> > +             }
> >       }
> >
> >       return;
> > --
> > 2.40.0
> >

Re: [PATCH] HID: wacom: Check for string overflow from strscpy calls

[Jiri Kosina]

On Fri, 14 Apr 2023, Jason Gerecke wrote:

> From: Jason Gerecke <killertofu@gmail.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: Jason Gerecke <jason.gerecke@wacom.com>
> Reviewed-by: Ping Cheng <ping.cheng@wacom.com>

Now applied, sorry for the delay.

-- 
Jiri Kosina
SUSE Labs