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

[Lee Jones]

On Mon, 02 Nov 2020, gregkh@xxxxxxxxxxxxxxxxxxx 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@xxxxxxxxxxxx>
> > > > Cc: "Eurotech S.p.A" <info@xxxxxxxxxxx>
> > > > Reported-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> > > > Acked-by: Arnd Bergmann <arnd@xxxxxxxx>
> > > > Signed-off-by: Lee Jones <lee.jones@xxxxxxxxxx>
> > > > ---
> > > >  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