linux-kernel - Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to

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: <CAHP4M8WaGt7q3t3z27df5BjHKNKsErjW-x-=awdoAvuq+jG77Q@mail.gmail.com>
Date:   Mon, 8 Nov 2021 17:42:54 +0530
From:   Ajay Garg <ajaygargnsit@...il.com>
To:     Pavel Skripkin <paskripkin@...il.com>
Cc:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Greg KH <gregkh@...uxfoundation.org>, jirislaby@...nel.org,
        kernel@...il.dk, David Laight <David.Laight@...lab.com>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user

Hi Pavel,

>
> Honestly, I can't get what you are trying to achieve with new string
> function.
>
> If caller knows, that there is no possible overflow, it can omit bounds
> checking (like in vt_do_kdgkb_ioctl). If caller needs return value equal
> to destination length it can use strscpy().

Please see the output corresponding for strscpy(), in the example output at
https://lore.kernel.org/linux-hardening/CAHP4M8U=0aTHgfREGJpSboV6J4X+E3Y6+H_kb-PvXxDKtV=n-g@mail.gmail.com/T/#m4a3f524eefe283a42430905fa4c0dfc2c37b2819

As is evident, even though destination length is 9, yet the returned
value is -7 (corresponding to -E2BIG).
So, strscpy() fails.

>
> There is a bunch of str*cpy() functions and every month I see new
> conversations between them on ML. As Andy said it's really chaos. These
> conversation are needed, of course, from security point of view, but
> lib/string is already big. It contains functions for every possible
> scenario, caller just needs to pick right one.

lib/string is big or small, that's not an excuse imho :)
I care about simplicity and easy lives for everyone in present (of
course) and future (more importantly).

As mentioned in :
https://lore.kernel.org/linux-hardening/CAHP4M8U=0aTHgfREGJpSboV6J4X+E3Y6+H_kb-PvXxDKtV=n-g@mail.gmail.com/T/#m4a3f524eefe283a42430905fa4c0dfc2c37b2819

there are several cases in files like fs/kernfs/dir.c, where
strlcpy()'s return value is directly propogated to the clients, and it
is not evident whether or not the return-value is within bounds.

If the new intended method is not added, we need to add checks in all
the clients (which is too much churn).

Instead, the new intended method will simplify lives for the clients,
when all they care is copy as much bytes as possible, and get the
number of bytes actualy copied.

It would be beneficial for all if discussions about the new method are
done on the intended thread.

Thanks and Regards,
Ajay