Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Date:   Fri, 29 Sep 2023 11:52:52 -0700
From:   Kees Cook <keescook@chromium.org>
To:     David Rheinsberg <david@readahead.eu>
Cc:     Justin Stitt <justinstitt@google.com>,
        Jiri Kosina <jikos@kernel.org>,
        Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-hardening@vger.kernel.org,
        David Herrmann <dh.herrmann@gmail.com>
Subject: Re: [PATCH] HID: uhid: refactor deprecated strncpy
Message-ID: <202309291151.11AFC5F83@keescook>
References: <20230914-strncpy-drivers-hid-uhid-c-v1-1-18a190060d8d@google.com>
 <202309142206.60836CE@keescook>
 <98d981a1-4e4c-4173-b8eb-09b4245bca60@app.fastmail.com>
 <202309151342.DFA6CA5C7@keescook>
 <72b7c13a-5f82-498b-84a3-b6e9b61c0e3a@app.fastmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <72b7c13a-5f82-498b-84a3-b6e9b61c0e3a@app.fastmail.com>
Precedence: bulk

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