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
