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 <[email protected]>
Cc: "Eurotech S.p.A" <[email protected]>
Reported-by: Geert Uytterhoeven <[email protected]>
Acked-by: Arnd Bergmann <[email protected]>
Signed-off-by: Lee Jones <[email protected]>
---
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));
c2dev->ops = ops;
mutex_init(&c2dev->mutex);
--
2.25.1
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 <[email protected]>
> > Cc: "Eurotech S.p.A" <[email protected]>
> > Reported-by: Geert Uytterhoeven <[email protected]>
> > Acked-by: Arnd Bergmann <[email protected]>
> > Signed-off-by: Lee Jones <[email protected]>
> > ---
> > 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/
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
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 <[email protected]>
> > > Cc: "Eurotech S.p.A" <[email protected]>
> > > Reported-by: Geert Uytterhoeven <[email protected]>
> > > Acked-by: Arnd Bergmann <[email protected]>
> > > Signed-off-by: Lee Jones <[email protected]>
> > > ---
> > > 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. But if you REALLY want to get it correct, call dev_set_name()
instead please, as that is what it is there for.
thanks,
greg k-h
On Mon, 02 Nov 2020, [email protected] 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 <[email protected]>
> > > > Cc: "Eurotech S.p.A" <[email protected]>
> > > > Reported-by: Geert Uytterhoeven <[email protected]>
> > > > Acked-by: Arnd Bergmann <[email protected]>
> > > > Signed-off-by: Lee Jones <[email protected]>
> > > > ---
> > > > 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.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, Nov 02, 2020 at 12:43:01PM +0000, Lee Jones wrote:
> On Mon, 02 Nov 2020, [email protected] 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 <[email protected]>
> > > > > Cc: "Eurotech S.p.A" <[email protected]>
> > > > > Reported-by: Geert Uytterhoeven <[email protected]>
> > > > > Acked-by: Arnd Bergmann <[email protected]>
> > > > > Signed-off-by: Lee Jones <[email protected]>
> > > > > ---
> > > > > 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.
thanks,
greg k-h
On Mon, 02 Nov 2020, [email protected] wrote:
> On Mon, Nov 02, 2020 at 12:43:01PM +0000, Lee Jones wrote:
> > On Mon, 02 Nov 2020, [email protected] 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 <[email protected]>
> > > > > > Cc: "Eurotech S.p.A" <[email protected]>
> > > > > > Reported-by: Geert Uytterhoeven <[email protected]>
> > > > > > Acked-by: Arnd Bergmann <[email protected]>
> > > > > > Signed-off-by: Lee Jones <[email protected]>
> > > > > > ---
> > > > > > 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.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
On 02/11/2020 14:47, Lee Jones wrote:
> On Mon, 02 Nov 2020, [email protected] wrote:
>
>> On Mon, Nov 02, 2020 at 12:43:01PM +0000, Lee Jones wrote:
>>> On Mon, 02 Nov 2020, [email protected] 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 <[email protected]>
>>>>>>> Cc: "Eurotech S.p.A" <[email protected]>
>>>>>>> Reported-by: Geert Uytterhoeven <[email protected]>
>>>>>>> Acked-by: Arnd Bergmann <[email protected]>
>>>>>>> Signed-off-by: Lee Jones <[email protected]>
>>>>>>> ---
>>>>>>> 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: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
On Mon, 02 Nov 2020, Rodolfo Giometti wrote:
> On 02/11/2020 14:47, Lee Jones wrote:
> > On Mon, 02 Nov 2020, [email protected] wrote:
> >
> >> On Mon, Nov 02, 2020 at 12:43:01PM +0000, Lee Jones wrote:
> >>> On Mon, 02 Nov 2020, [email protected] 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 <[email protected]>
> >>>>>>> Cc: "Eurotech S.p.A" <[email protected]>
> >>>>>>> Reported-by: Geert Uytterhoeven <[email protected]>
> >>>>>>> Acked-by: Arnd Bergmann <[email protected]>
> >>>>>>> Signed-off-by: Lee Jones <[email protected]>
> >>>>>>> ---
> >>>>>>> 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. :)
If you're sure, I can add it to my TODO.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
On 03/11/2020 09:57, Lee Jones wrote:
> On Mon, 02 Nov 2020, Rodolfo Giometti wrote:
>
>> On 02/11/2020 14:47, Lee Jones wrote:
>>> On Mon, 02 Nov 2020, [email protected] wrote:
>>>
>>>> On Mon, Nov 02, 2020 at 12:43:01PM +0000, Lee Jones wrote:
>>>>> On Mon, 02 Nov 2020, [email protected] 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 <[email protected]>
>>>>>>>>> Cc: "Eurotech S.p.A" <[email protected]>
>>>>>>>>> Reported-by: Geert Uytterhoeven <[email protected]>
>>>>>>>>> Acked-by: Arnd Bergmann <[email protected]>
>>>>>>>>> Signed-off-by: Lee Jones <[email protected]>
>>>>>>>>> ---
>>>>>>>>> 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. :)
>
> If you're sure, I can add it to my TODO.
Yes, no problem!
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti