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 >
