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
| ||
|
Message-ID: <202309291151.11AFC5F83@keescook> Date: Fri, 29 Sep 2023 11:52:52 -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 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 -- Kees Cook
Powered by blists - more mailing lists