diff mbox series

HID: uhid: refactor deprecated strncpy

Message ID 20230914-strncpy-drivers-hid-uhid-c-v1-1-18a190060d8d@google.com (mailing list archive)
State Changes Requested
Headers show
Series HID: uhid: refactor deprecated strncpy | expand

Commit Message

Justin Stitt Sept. 14, 2023, 10:47 p.m. UTC
`strncpy` is deprecated for use on NUL-terminated destination strings [1].

We should prefer more robust and less ambiguous string interfaces.

A suitable replacement is `strscpy` [2] due to the fact that it
guarantees NUL-termination on the destination buffer without
unnecessarily NUL-padding.

Looking at: Commit 4d26d1d1e806 ("Revert "HID: uhid: use strlcpy() instead of strncpy()"")
we see referenced the fact that many attempts have been made to change
these strncpy's into strlcpy to no success. I think strscpy is an
objectively better interface here as it doesn't unnecessarily NUL-pad
the destination buffer whilst allowing us to drop the `len = min(...)`
pattern as strscpy will implicitly limit the number of bytes copied by
the smaller of its dest and src arguments.

So while the existing code may not have a bug (i.e: overread problems)
we should still favor strscpy due to readability (plus a very slight
performance boost).

Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-hardening@vger.kernel.org
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
 drivers/hid/uhid.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)


---
base-commit: 3669558bdf354cd352be955ef2764cde6a9bf5ec
change-id: 20230914-strncpy-drivers-hid-uhid-c-a465f3e784dd

Best regards,
--
Justin Stitt <justinstitt@google.com>

Comments

Kees Cook Sept. 15, 2023, 5:13 a.m. UTC | #1
On Thu, Sep 14, 2023 at 10:47:21PM +0000, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> 
> We should prefer more robust and less ambiguous string interfaces.
> 
> A suitable replacement is `strscpy` [2] due to the fact that it
> guarantees NUL-termination on the destination buffer without
> unnecessarily NUL-padding.
> 
> Looking at: Commit 4d26d1d1e806 ("Revert "HID: uhid: use strlcpy() instead of strncpy()"")
> we see referenced the fact that many attempts have been made to change
> these strncpy's into strlcpy to no success. I think strscpy is an
> objectively better interface here as it doesn't unnecessarily NUL-pad
> the destination buffer whilst allowing us to drop the `len = min(...)`
> pattern as strscpy will implicitly limit the number of bytes copied by
> the smaller of its dest and src arguments.

I think the worry here is that ev->u.create2.name may not be %NUL
terminated. But I think that doesn't matter, see below...

Also, note, this should CC David Herrmann <dh.herrmann@gmail.com>
(now added).

> So while the existing code may not have a bug (i.e: overread problems)
> we should still favor strscpy due to readability (plus a very slight
> performance boost).
> 
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
>  drivers/hid/uhid.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> index 4588d2cd4ea4..00e1566ad37b 100644
> --- a/drivers/hid/uhid.c
> +++ b/drivers/hid/uhid.c
> @@ -490,7 +490,7 @@ static int uhid_dev_create2(struct uhid_device *uhid,
>  			    const struct uhid_event *ev)
>  {
>  	struct hid_device *hid;
> -	size_t rd_size, len;
> +	size_t rd_size;
>  	void *rd_data;
>  	int ret;
>  
> @@ -514,13 +514,9 @@ static int uhid_dev_create2(struct uhid_device *uhid,
>  		goto err_free;
>  	}
>  
> -	/* @hid is zero-initialized, strncpy() is correct, strlcpy() not */
> -	len = min(sizeof(hid->name), sizeof(ev->u.create2.name)) - 1;
> -	strncpy(hid->name, ev->u.create2.name, len);
> -	len = min(sizeof(hid->phys), sizeof(ev->u.create2.phys)) - 1;
> -	strncpy(hid->phys, ev->u.create2.phys, len);
> -	len = min(sizeof(hid->uniq), sizeof(ev->u.create2.uniq)) - 1;
> -	strncpy(hid->uniq, ev->u.create2.uniq, len);

ev->u.create2 is:
struct uhid_create2_req {
        __u8 name[128];
        __u8 phys[64];
        __u8 uniq[64];
	...

hid is:
struct hid_device { /* device report descriptor */
	...
        char name[128]; /* Device name */
        char phys[64]; /* Device physical location */
        char uniq[64]; /* Device unique identifier (serial #) */

So these "min" calls are redundant -- it wants to copy at most 1 less so
it can be %NUL terminated. Which is what strscpy() already does. And
source and dest are the same size, so we can't over-read source if it
weren't terminated (since strscpy won't overread like strlcpy).

> +	strscpy(hid->name, ev->u.create2.name, sizeof(hid->name));
> +	strscpy(hid->phys, ev->u.create2.phys, sizeof(hid->phys));
> +	strscpy(hid->uniq, ev->u.create2.uniq, sizeof(hid->uniq));

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

>  
>  	hid->ll_driver = &uhid_hid_driver;
>  	hid->bus = ev->u.create2.bus;
> 
> ---
> base-commit: 3669558bdf354cd352be955ef2764cde6a9bf5ec
> change-id: 20230914-strncpy-drivers-hid-uhid-c-a465f3e784dd
> 
> Best regards,
> --
> Justin Stitt <justinstitt@google.com>
>
David Rheinsberg Sept. 15, 2023, 7:36 a.m. UTC | #2
Hi

On Fri, Sep 15, 2023, at 7:13 AM, Kees Cook wrote:
>> -	/* @hid is zero-initialized, strncpy() is correct, strlcpy() not */
>> -	len = min(sizeof(hid->name), sizeof(ev->u.create2.name)) - 1;
>> -	strncpy(hid->name, ev->u.create2.name, len);
>> -	len = min(sizeof(hid->phys), sizeof(ev->u.create2.phys)) - 1;
>> -	strncpy(hid->phys, ev->u.create2.phys, len);
>> -	len = min(sizeof(hid->uniq), sizeof(ev->u.create2.uniq)) - 1;
>> -	strncpy(hid->uniq, ev->u.create2.uniq, len);
>
> ev->u.create2 is:
> struct uhid_create2_req {
>         __u8 name[128];
>         __u8 phys[64];
>         __u8 uniq[64];
> 	...
>
> hid is:
> struct hid_device { /* device report descriptor */
> 	...
>         char name[128]; /* Device name */
>         char phys[64]; /* Device physical location */
>         char uniq[64]; /* Device unique identifier (serial #) */
>
> So these "min" calls are redundant -- it wants to copy at most 1 less so
> it can be %NUL terminated. Which is what strscpy() already does. And
> source and dest are the same size, so we can't over-read source if it
> weren't terminated (since strscpy won't overread like strlcpy).

I *really* think we should keep the `min` calls. The compiler should already optimize them away, as both arguments are compile-time constants. There is no inherent reason why source and target are equal in size. Yes, it is unlikely to change, but I don't understand why we would want to implicitly rely on it, rather than make the compiler verify it for us. And `struct hid_device` is very much allowed to change in the future.

As an alternative, you can use BUILD_BUG_ON() and verify both are equal in length.

Thanks
David
Kees Cook Sept. 15, 2023, 8:48 p.m. UTC | #3
On Fri, Sep 15, 2023 at 09:36:23AM +0200, David Rheinsberg wrote:
> Hi
> 
> On Fri, Sep 15, 2023, at 7:13 AM, Kees Cook wrote:
> >> -	/* @hid is zero-initialized, strncpy() is correct, strlcpy() not */
> >> -	len = min(sizeof(hid->name), sizeof(ev->u.create2.name)) - 1;
> >> -	strncpy(hid->name, ev->u.create2.name, len);
> >> -	len = min(sizeof(hid->phys), sizeof(ev->u.create2.phys)) - 1;
> >> -	strncpy(hid->phys, ev->u.create2.phys, len);
> >> -	len = min(sizeof(hid->uniq), sizeof(ev->u.create2.uniq)) - 1;
> >> -	strncpy(hid->uniq, ev->u.create2.uniq, len);
> >
> > ev->u.create2 is:
> > struct uhid_create2_req {
> >         __u8 name[128];
> >         __u8 phys[64];
> >         __u8 uniq[64];
> > 	...
> >
> > hid is:
> > struct hid_device { /* device report descriptor */
> > 	...
> >         char name[128]; /* Device name */
> >         char phys[64]; /* Device physical location */
> >         char uniq[64]; /* Device unique identifier (serial #) */
> >
> > So these "min" calls are redundant -- it wants to copy at most 1 less so
> > it can be %NUL terminated. Which is what strscpy() already does. And
> > source and dest are the same size, so we can't over-read source if it
> > weren't terminated (since strscpy won't overread like strlcpy).
> 
> I *really* think we should keep the `min` calls. The compiler
> should already optimize them away, as both arguments are compile-time
> constants. There is no inherent reason why source and target are equal in
> size. Yes, it is unlikely to change, but I don't understand why we would
> want to implicitly rely on it, rather than make the compiler verify it for
> us. And `struct hid_device` is very much allowed to change in the future.
> 
> As an alternative, you can use BUILD_BUG_ON() and verify both are equal in length.

If we can't depend on ev->u.create2.name/phys/uniq being %NUL-terminated,
we've already done the "min" calculations, and we've already got the
dest zeroed, then I suspect the thing to do is just use memcpy instead
of strncpy (or strscpy).
David Rheinsberg Sept. 18, 2023, 7:37 a.m. UTC | #4
Hey

On Fri, Sep 15, 2023, at 10:48 PM, Kees Cook wrote:
> On Fri, Sep 15, 2023 at 09:36:23AM +0200, David Rheinsberg wrote:
>> Hi
>> 
>> On Fri, Sep 15, 2023, at 7:13 AM, Kees Cook wrote:
>> >> -	/* @hid is zero-initialized, strncpy() is correct, strlcpy() not */
>> >> -	len = min(sizeof(hid->name), sizeof(ev->u.create2.name)) - 1;
>> >> -	strncpy(hid->name, ev->u.create2.name, len);
>> >> -	len = min(sizeof(hid->phys), sizeof(ev->u.create2.phys)) - 1;
>> >> -	strncpy(hid->phys, ev->u.create2.phys, len);
>> >> -	len = min(sizeof(hid->uniq), sizeof(ev->u.create2.uniq)) - 1;
>> >> -	strncpy(hid->uniq, ev->u.create2.uniq, len);
>> >
>> > ev->u.create2 is:
>> > struct uhid_create2_req {
>> >         __u8 name[128];
>> >         __u8 phys[64];
>> >         __u8 uniq[64];
>> > 	...
>> >
>> > hid is:
>> > struct hid_device { /* device report descriptor */
>> > 	...
>> >         char name[128]; /* Device name */
>> >         char phys[64]; /* Device physical location */
>> >         char uniq[64]; /* Device unique identifier (serial #) */
>> >
>> > So these "min" calls are redundant -- it wants to copy at most 1 less so
>> > it can be %NUL terminated. Which is what strscpy() already does. And
>> > source and dest are the same size, so we can't over-read source if it
>> > weren't terminated (since strscpy won't overread like strlcpy).
>> 
>> I *really* think we should keep the `min` calls. The compiler
>> should already optimize them away, as both arguments are compile-time
>> constants. There is no inherent reason why source and target are equal in
>> size. Yes, it is unlikely to change, but I don't understand why we would
>> want to implicitly rely on it, rather than make the compiler verify it for
>> us. And `struct hid_device` is very much allowed to change in the future.
>> 
>> As an alternative, you can use BUILD_BUG_ON() and verify both are equal in length.
>
> If we can't depend on ev->u.create2.name/phys/uniq being %NUL-terminated,
> we've already done the "min" calculations, and we've already got the
> dest zeroed, then I suspect the thing to do is just use memcpy instead
> of strncpy (or strscpy).

If you use memcpy, you might copy garbage trailing the terminating zero. This is not particularly wrong, but also not really nice if user-space relies on the kernel to treat it as a string. You don't know whether a query of the string returns trailing bytes, and thus might expose data that user-space did not intend to share.

I mean, this is why the code uses strncpy().

Thanks
David
Kees Cook Sept. 29, 2023, 6:52 p.m. UTC | #5
On Mon, Sep 18, 2023 at 09:37:53AM +0200, David Rheinsberg wrote:
> Hey
> 
> On Fri, Sep 15, 2023, at 10:48 PM, Kees Cook wrote:
> > On Fri, Sep 15, 2023 at 09:36:23AM +0200, David Rheinsberg wrote:
> >> Hi
> >> 
> >> On Fri, Sep 15, 2023, at 7:13 AM, Kees Cook wrote:
> >> >> -	/* @hid is zero-initialized, strncpy() is correct, strlcpy() not */
> >> >> -	len = min(sizeof(hid->name), sizeof(ev->u.create2.name)) - 1;
> >> >> -	strncpy(hid->name, ev->u.create2.name, len);
> >> >> -	len = min(sizeof(hid->phys), sizeof(ev->u.create2.phys)) - 1;
> >> >> -	strncpy(hid->phys, ev->u.create2.phys, len);
> >> >> -	len = min(sizeof(hid->uniq), sizeof(ev->u.create2.uniq)) - 1;
> >> >> -	strncpy(hid->uniq, ev->u.create2.uniq, len);
> >> >
> >> > ev->u.create2 is:
> >> > struct uhid_create2_req {
> >> >         __u8 name[128];
> >> >         __u8 phys[64];
> >> >         __u8 uniq[64];
> >> > 	...
> >> >
> >> > hid is:
> >> > struct hid_device { /* device report descriptor */
> >> > 	...
> >> >         char name[128]; /* Device name */
> >> >         char phys[64]; /* Device physical location */
> >> >         char uniq[64]; /* Device unique identifier (serial #) */
> >> >
> >> > So these "min" calls are redundant -- it wants to copy at most 1 less so
> >> > it can be %NUL terminated. Which is what strscpy() already does. And
> >> > source and dest are the same size, so we can't over-read source if it
> >> > weren't terminated (since strscpy won't overread like strlcpy).
> >> 
> >> I *really* think we should keep the `min` calls. The compiler
> >> should already optimize them away, as both arguments are compile-time
> >> constants. There is no inherent reason why source and target are equal in
> >> size. Yes, it is unlikely to change, but I don't understand why we would
> >> want to implicitly rely on it, rather than make the compiler verify it for
> >> us. And `struct hid_device` is very much allowed to change in the future.
> >> 
> >> As an alternative, you can use BUILD_BUG_ON() and verify both are equal in length.
> >
> > If we can't depend on ev->u.create2.name/phys/uniq being %NUL-terminated,
> > we've already done the "min" calculations, and we've already got the
> > dest zeroed, then I suspect the thing to do is just use memcpy instead
> > of strncpy (or strscpy).
> 
> If you use memcpy, you might copy garbage trailing the terminating zero. This is not particularly wrong, but also not really nice if user-space relies on the kernel to treat it as a string. You don't know whether a query of the string returns trailing bytes, and thus might expose data that user-space did not intend to share.
> 
> I mean, this is why the code uses strncpy().

Justin, can you respin this patch (with an updated Subject and commit
log), and add BUILD_BUG_ON() to verify the sizes are the same in addition
to what you already had in the original patch?

I think that'll give us the right balance between correctness,
readability, and future-proofing.

-Kees
diff mbox series

Patch

diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 4588d2cd4ea4..00e1566ad37b 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -490,7 +490,7 @@  static int uhid_dev_create2(struct uhid_device *uhid,
 			    const struct uhid_event *ev)
 {
 	struct hid_device *hid;
-	size_t rd_size, len;
+	size_t rd_size;
 	void *rd_data;
 	int ret;
 
@@ -514,13 +514,9 @@  static int uhid_dev_create2(struct uhid_device *uhid,
 		goto err_free;
 	}
 
-	/* @hid is zero-initialized, strncpy() is correct, strlcpy() not */
-	len = min(sizeof(hid->name), sizeof(ev->u.create2.name)) - 1;
-	strncpy(hid->name, ev->u.create2.name, len);
-	len = min(sizeof(hid->phys), sizeof(ev->u.create2.phys)) - 1;
-	strncpy(hid->phys, ev->u.create2.phys, len);
-	len = min(sizeof(hid->uniq), sizeof(ev->u.create2.uniq)) - 1;
-	strncpy(hid->uniq, ev->u.create2.uniq, len);
+	strscpy(hid->name, ev->u.create2.name, sizeof(hid->name));
+	strscpy(hid->phys, ev->u.create2.phys, sizeof(hid->phys));
+	strscpy(hid->uniq, ev->u.create2.uniq, sizeof(hid->uniq));
 
 	hid->ll_driver = &uhid_hid_driver;
 	hid->bus = ev->u.create2.bus;