[PATCH 1/2] misc: c2port: core: Make copying name from userspace more secure

[PATCH 1/2] misc: c2port: core: Make copying name from userspace more secure

[Lee Jones]

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 <giometti@enneenne.com>
Cc: "Eurotech S.p.A" <info@eurotech.it>
Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Lee Jones <lee.jones@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));
 	c2dev->ops = ops;
 	mutex_init(&c2dev->mutex);
 
-- 
2.25.1

[PATCH 2/2] misc: ocxl: config: Rename function attribute description

[Lee Jones]

Fixes the following W=1 kernel build warning(s):

 drivers/misc/ocxl/config.c:81: warning: Function parameter or member 'dev' not described in 'get_function_0'
 drivers/misc/ocxl/config.c:81: warning: Excess function parameter 'device' description in 'get_function_0'

Cc: Frederic Barrat <fbarrat@linux.ibm.com>
Cc: Andrew Donnellan <ajd@linux.ibm.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/misc/ocxl/config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/ocxl/config.c b/drivers/misc/ocxl/config.c
index 4d490b92d951f..a68738f382521 100644
--- a/drivers/misc/ocxl/config.c
+++ b/drivers/misc/ocxl/config.c
@@ -73,7 +73,7 @@ static int find_dvsec_afu_ctrl(struct pci_dev *dev, u8 afu_idx)
 
 /**
  * get_function_0() - Find a related PCI device (function 0)
- * @device: PCI device to match
+ * @dev: PCI device to match
  *
  * Returns a pointer to the related device, or null if not found
  */
-- 
2.25.1

Re: [PATCH 1/2] misc: c2port: core: Make copying name from userspace more secure

[Lee Jones]

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 <giometti@enneenne.com>
> > Cc: "Eurotech S.p.A" <info@eurotech.it>
> > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Acked-by: Arnd Bergmann <arnd@arndb.de>
> > Signed-off-by: Lee Jones <lee.jones@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/

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/2] misc: c2port: core: Make copying name from userspace more secure

[gregkh]

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 <giometti@enneenne.com>
> > > Cc: "Eurotech S.p.A" <info@eurotech.it>
> > > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > Acked-by: Arnd Bergmann <arnd@arndb.de>
> > > Signed-off-by: Lee Jones <lee.jones@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.  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

Re: [PATCH 1/2] misc: c2port: core: Make copying name from userspace more secure

[Lee Jones]

On Mon, 02 Nov 2020, gregkh@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 <giometti@enneenne.com>
> > > > Cc: "Eurotech S.p.A" <info@eurotech.it>
> > > > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > Acked-by: Arnd Bergmann <arnd@arndb.de>
> > > > Signed-off-by: Lee Jones <lee.jones@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.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/2] misc: c2port: core: Make copying name from userspace more secure

[gregkh]

On Mon, Nov 02, 2020 at 12:43:01PM +0000, Lee Jones wrote:
> On Mon, 02 Nov 2020, gregkh@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 <giometti@enneenne.com>
> > > > > Cc: "Eurotech S.p.A" <info@eurotech.it>
> > > > > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > > Acked-by: Arnd Bergmann <arnd@arndb.de>
> > > > > Signed-off-by: Lee Jones <lee.jones@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.

thanks,

greg k-h

Re: [PATCH 1/2] misc: c2port: core: Make copying name from userspace more secure

[Lee Jones]

On Mon, 02 Nov 2020, gregkh@linuxfoundation.org wrote:

> On Mon, Nov 02, 2020 at 12:43:01PM +0000, Lee Jones wrote:
> > On Mon, 02 Nov 2020, gregkh@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 <giometti@enneenne.com>
> > > > > > Cc: "Eurotech S.p.A" <info@eurotech.it>
> > > > > > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > > > Acked-by: Arnd Bergmann <arnd@arndb.de>
> > > > > > Signed-off-by: Lee Jones <lee.jones@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.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/2] misc: c2port: core: Make copying name from userspace more secure

[Rodolfo Giometti]

On 02/11/2020 14:47, Lee Jones wrote:
> On Mon, 02 Nov 2020, gregkh@linuxfoundation.org wrote:
> 
>> On Mon, Nov 02, 2020 at 12:43:01PM +0000, Lee Jones wrote:
>>> On Mon, 02 Nov 2020, gregkh@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 <giometti@enneenne.com>
>>>>>>> Cc: "Eurotech S.p.A" <info@eurotech.it>
>>>>>>> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>>>>> Acked-by: Arnd Bergmann <arnd@arndb.de>
>>>>>>> Signed-off-by: Lee Jones <lee.jones@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: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

Re: [PATCH 1/2] misc: c2port: core: Make copying name from userspace more secure

[Lee Jones]

On Mon, 02 Nov 2020, Rodolfo Giometti wrote:

> On 02/11/2020 14:47, Lee Jones wrote:
> > On Mon, 02 Nov 2020, gregkh@linuxfoundation.org wrote:
> > 
> >> On Mon, Nov 02, 2020 at 12:43:01PM +0000, Lee Jones wrote:
> >>> On Mon, 02 Nov 2020, gregkh@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 <giometti@enneenne.com>
> >>>>>>> Cc: "Eurotech S.p.A" <info@eurotech.it>
> >>>>>>> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >>>>>>> Acked-by: Arnd Bergmann <arnd@arndb.de>
> >>>>>>> Signed-off-by: Lee Jones <lee.jones@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. :)

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

Re: [PATCH 1/2] misc: c2port: core: Make copying name from userspace more secure

[Rodolfo Giometti]

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, gregkh@linuxfoundation.org wrote:
>>>
>>>> On Mon, Nov 02, 2020 at 12:43:01PM +0000, Lee Jones wrote:
>>>>> On Mon, 02 Nov 2020, gregkh@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 <giometti@enneenne.com>
>>>>>>>>> Cc: "Eurotech S.p.A" <info@eurotech.it>
>>>>>>>>> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>>>>>>> Acked-by: Arnd Bergmann <arnd@arndb.de>
>>>>>>>>> Signed-off-by: Lee Jones <lee.jones@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. :)
> 
> If you're sure, I can add it to my TODO.

Yes, no problem!

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti