linux-kernel - Re: [PATCH] HID: uhid: refactor deprecated strncpy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <202309151342.DFA6CA5C7@keescook>
Date:   Fri, 15 Sep 2023 13:48:23 -0700
From:   Kees Cook <keescook@...omium.org>
To:     David Rheinsberg <david@...dahead.eu>
Cc:     Justin Stitt <justinstitt@...gle.com>,
        Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-hardening@...r.kernel.org,
        David Herrmann <dh.herrmann@...il.com>
Subject: Re: [PATCH] HID: uhid: refactor deprecated strncpy

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

-- 
Kees Cook