Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Feedback-ID: id2994666:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-745-g95dd7bea33-fm-20230905.001-g95dd7bea
Mime-Version: 1.0
Message-Id: <98d981a1-4e4c-4173-b8eb-09b4245bca60@app.fastmail.com>
In-Reply-To: <202309142206.60836CE@keescook>
References: <20230914-strncpy-drivers-hid-uhid-c-v1-1-18a190060d8d@google.com>
 <202309142206.60836CE@keescook>
Date:   Fri, 15 Sep 2023 09:36:23 +0200
From:   "David Rheinsberg" <david@readahead.eu>
To:     "Kees Cook" <keescook@chromium.org>,
        "Justin Stitt" <justinstitt@google.com>
Cc:     "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
Content-Type: text/plain
Precedence: bulk

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