Re: [PATCH] drivers/input: Remove all strcpy() uses in favor of strscpy()

[Hans Verkuil]

On 03/08/2021 09:07, Kees Cook wrote:
> On Sun, Aug 01, 2021 at 09:44:33AM -0700, Kees Cook wrote:
>> On Sun, Aug 01, 2021 at 05:57:32PM +0200, Len Baker wrote:
>>> Hi,
>>>
>>> On Sun, Aug 01, 2021 at 04:00:00PM +0100, Russell King (Oracle) wrote:
>>>> On Sun, Aug 01, 2021 at 04:43:16PM +0200, Len Baker wrote:
>>>>> strcpy() performs no bounds checking on the destination buffer. This
>>>>> could result in linear overflows beyond the end of the buffer, leading
>>>>> to all kinds of misbehaviors. The safe replacement is strscpy().
>>>>>
>>>>> Signed-off-by: Len Baker <len.baker@xxxxxxx>
>>>>> ---
>>>>> This is a task of the KSPP [1]
>>>>>
>>>>> [1] https://github.com/KSPP/linux/issues/88
>>>>>
>>>>>  drivers/input/keyboard/locomokbd.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/input/keyboard/locomokbd.c b/drivers/input/keyboard/locomokbd.c
>>>>> index dae053596572..dbb3dc48df12 100644
>>>>> --- a/drivers/input/keyboard/locomokbd.c
>>>>> +++ b/drivers/input/keyboard/locomokbd.c
>>>>> @@ -254,7 +254,7 @@ static int locomokbd_probe(struct locomo_dev *dev)
>>>>>  	locomokbd->suspend_jiffies = jiffies;
>>>>>
>>>>>  	locomokbd->input = input_dev;
>>>>> -	strcpy(locomokbd->phys, "locomokbd/input0");
>>>>> +	strscpy(locomokbd->phys, "locomokbd/input0", sizeof(locomokbd->phys));
>>>>
>>>> So if the string doesn't fit, it's fine to silently truncate it?
>>>
>>> I think it is better than overflow :)
>>>
>>>> Rather than converting every single strcpy() in the kernel to
>>>> strscpy(), maybe there should be some consideration given to how the
>>>> issue of a strcpy() that overflows the buffer should be handled.
>>>> E.g. in the case of a known string such as the above, if it's longer
>>>> than the destination, should we find a way to make the compiler issue
>>>> a warning at compile time?
>>>
>>> Good point. I am a kernel newbie and have no experience. So this
>>> question should be answered by some kernel hacker :) But I agree
>>> with your proposals.
>>>
>>> Kees and folks: Any comments?
>>>
>>> Note: Kees is asked the same question in [2]
>>>
>>> [2] https://lore.kernel.org/lkml/20210731135957.GB1979@titan/
>>
>> Hi!
>>
>> Sorry for the delay at looking into this. It didn't use to be a problem
>> (there would always have been a compile-time warning generated for
>> known-too-small cases), but that appears to have regressed when,
>> ironically, strscpy() coverage was added. I've detailed it in the bug
>> report:
>> https://github.com/KSPP/linux/issues/88
>>
>> So, bottom line: we need to fix the missing compile-time warnings for
>> strcpy() and strscpy() under CONFIG_FORTIFY_SOURCE=y.
> 
> I've got these fixed now, and will send them out likely tomorrow, but I
> did, in fact, find 4 cases of truncation, all in v4l, and all appear to
> have been truncated since introduction:
> 
> struct v4l2_capability {
> ...
>         __u8    card[32];
> (stores 31 characters)
> 
> drivers/media/radio/radio-wl1273.c:1282
> wl1273_fm_vidioc_querycap()
>             strscpy(capability->card, "Texas Instruments Wl1273 FM Radio",
>                     sizeof(capability->card));
> 33 characters, getting truncated to:
> Texas Instruments Wl1273 FM Rad
> 87d1a50ce45168cbaec10397e876286a398052c1

I'd change this to: "TI WL1273 FM Radio"

> 
> drivers/media/radio/si470x/radio-si470x-usb.c:514
> si470x_vidioc_querycap()
> #define DRIVER_CARD "Silicon Labs Si470x FM Radio Receiver"
>             strscpy(capability->card, DRIVER_CARD,
> sizeof(capability->card));
> 37 characters, getting truncated to:
> Silicon Labs Si470x FM Radio Re
> 78656acdcf4852547a29e929a1b7a98d5ac65f17

This to "Silicon Labs Si470x FM Radio"

> 
> drivers/media/radio/si470x/radio-si470x-i2c.c:225
> si470x_vidioc_querycap()
> #define DRIVER_CARD "Silicon Labs Si470x FM Radio Receiver"
>             strscpy(capability->card, DRIVER_CARD,
> sizeof(capability->card));
> 37 characters, getting truncated to:
> Silicon Labs Si470x FM Radio Re
> cc35bbddfe10f77d949f0190764b252cd2b70c3c

Ditto.

> 
> drivers/media/usb/tm6000/tm6000-video.c:855
> vidioc_querycap()
>             strscpy(cap->card, "Trident TVMaster TM5600/6000/6010",
>                     sizeof(cap->card));
> 33 characters, getting truncated to:
> Trident TVMaster TM5600/6000/60
> e28f49b0b2a8e678af62745ffdc4e4f36d7283a6

And this to: "Trident TM5600/6000/6010"

The truncation doesn't hurt anything, it's just looks a bit ugly.
These shorter names should solve this issue.

Regards,

	Hans

> 
> How should these be handled? I assume v4l2_capability::card can't be
> resized since it's part of IOCTL response, so likely all the string just
> need to be shortened in some way? Seems like dropping the manufacturer
> name makes the most sense, since manufacturer can be kind of derived
> from the driver names.
> 
> Thoughts?
> 
> -Kees
>