Messages in this thread | | | Date | Thu, 23 Jul 2020 09:41:15 +0200 | From | Greg KH <> | Subject | Re: [PATCH 1/1] misc: c2port: core: Make copying name from userspace more secure |
| |
On Tue, Jul 14, 2020 at 09:32:59AM +0100, Lee Jones wrote: > Currently the 'c2dev' device data is not zeroed when its allocated. > Coupled with the fact strncpy() *may not* provide a NUL terminator > means that a 1-byte leak would be possible *if* this was ever copied > to userspace.
c2dev is a kernel internal structure, it is never copied to userspace, so why even mention such a thing?
> To prevent such a failing, let's first ensure the 'c2dev' device data > area is fully zeroed out and 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> > Signed-off-by: Lee Jones <lee.jones@linaro.org> > --- > drivers/misc/c2port/core.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/misc/c2port/core.c b/drivers/misc/c2port/core.c > index 80d87e8a0bea9..0de538a1cc1c6 100644 > --- a/drivers/misc/c2port/core.c > +++ b/drivers/misc/c2port/core.c > @@ -899,7 +899,7 @@ struct c2port_device *c2port_device_register(char *name, > unlikely(!ops->c2d_get) || unlikely(!ops->c2d_set)) > return ERR_PTR(-EINVAL); > > - c2dev = kmalloc(sizeof(struct c2port_device), GFP_KERNEL); > + c2dev = kzalloc(sizeof(struct c2port_device), GFP_KERNEL);
All fields seem to be properly initialized so this really isn't needed from what I can tell.
> if (unlikely(!c2dev)) > return ERR_PTR(-ENOMEM); > > @@ -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));
Given there is only 1 user of this function, and it passes in "uc", this isn't a big deal :)
But, I can take this as a separate patch, if you want to redo this, just to be safe in the future.
thanks,
greg k-h
| |