On 02/11/2020 14:47, Lee Jones wrote: > On Mon, 02 Nov 2020, gre...@linuxfoundation.org wrote: > >> On Mon, Nov 02, 2020 at 12:43:01PM +0000, Lee Jones wrote: >>> On Mon, 02 Nov 2020, gre...@linuxfoundation.org wrote: >>> >>>> On Mon, Nov 02, 2020 at 11:49:03AM +0000, Lee Jones wrote: >>>>> On Mon, 02 Nov 2020, David Laight wrote: >>>>> >>>>>> From: Lee Jones >>>>>>> Sent: 02 November 2020 11:12 >>>>>>> >>>>>>> strncpy() may not provide a NUL terminator, which means that a 1-byte >>>>>>> leak would be possible *if* this was ever copied to userspace. Ensure >>>>>>> the buffer will always be NUL terminated by using the kernel's >>>>>>> strscpy() which a) uses the destination (instead of the source) size >>>>>>> as the bytes to copy and b) is *always* NUL terminated. >>>>>>> >>>>>>> Cc: Rodolfo Giometti <giome...@enneenne.com> >>>>>>> Cc: "Eurotech S.p.A" <i...@eurotech.it> >>>>>>> Reported-by: Geert Uytterhoeven <geert+rene...@glider.be> >>>>>>> Acked-by: Arnd Bergmann <a...@arndb.de> >>>>>>> Signed-off-by: Lee Jones <lee.jo...@linaro.org> >>>>>>> --- >>>>>>> drivers/misc/c2port/core.c | 2 +- >>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/misc/c2port/core.c b/drivers/misc/c2port/core.c >>>>>>> index 80d87e8a0bea9..b96444ec94c7e 100644 >>>>>>> --- a/drivers/misc/c2port/core.c >>>>>>> +++ b/drivers/misc/c2port/core.c >>>>>>> @@ -923,7 +923,7 @@ struct c2port_device *c2port_device_register(char >>>>>>> *name, >>>>>>> } >>>>>>> dev_set_drvdata(c2dev->dev, c2dev); >>>>>>> >>>>>>> - strncpy(c2dev->name, name, C2PORT_NAME_LEN - 1); >>>>>>> + strscpy(c2dev->name, name, sizeof(c2dev->name)); >>>>>> >>>>>> strscpy() doesn't zero fill so if the memory isn't zeroed >>>>>> and a 'blind' copy to user of the structure is done >>>>>> then more data is leaked. >>>>>> >>>>>> strscpy() may be better, but rational isn't right. >>>>> >>>>> The original patch zeroed the data too, but I was asked to remove that >>>>> part [0]. In your opinion, should it be reinstated? >>>>> >>>>> [0] https://lore.kernel.org/patchwork/patch/1272290/ >>>> >>>> Just keep the kzalloc() part of the patch, this portion makes no sense >>>> to me. >>> >>> Can do. >>> >>>> But if you REALLY want to get it correct, call dev_set_name() >>>> instead please, as that is what it is there for. >>> >>> The line above isn't setting the 'struct device' name. It looks as >>> though 'struct c2port' has it's own member, also called 'name'. As to >>> how they differ, I'm not currently aware. Nor do I wish to mess >>> around with the semantics all that much. >>> >>> Going with suggestion #1. >> >> As the "device" already has a name, I suggest just getting rid of this >> name field anyway, no need for duplicates. > > That definitely goes against the point I made above: > > "Nor do I wish to mess around with the semantics all that much." > > It looks as though the device name 'c2port%d' varies greatly to the > requested name 'uc'. I don't have enough knowledge of how user- > space expects to use the provided sysfs entries to be able to > competently merge/decide which of these should be kept and which to > discard. > > Hopefully one of the authors/maintainers are reading this and can come > up with an acceptable solution.
User-space usage can change its behavior so, please, consider the best solution from the kernel space point-of-view. :) Ciao, Rodolfo -- GNU/Linux Solutions e-mail: giome...@enneenne.com Linux Device Driver giome...@linux.it Embedded Systems phone: +39 349 2432127 UNIX programming skype: rodolfo.giometti
