On Wed, Jun 10, 2015 at 12:41:23AM -0400, green@xxxxxxxxxxxxxx wrote: > From: Oleg Drokin <green@xxxxxxxxxxxxxx> > > strncpy_from_user could return negative values on error, > so need to take those into account. > Since ll_getname is used to get a single component name from userspace > to transfer to server as-is, there's no need to allocate 4k buffer > as done by __getname. Allocate NAME_MAX+1 buffer instead to ensure > we have enough for a null terminated max valid length buffer. > > This was discovered by Al Viro in https://lkml.org/lkml/2015/4/11/243 > > Signed-off-by: Oleg Drokin <green@xxxxxxxxxxxxxx> > --- > drivers/staging/lustre/lustre/llite/dir.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/drivers/staging/lustre/lustre/llite/dir.c b/drivers/staging/lustre/lustre/llite/dir.c > index 87a042c..e0b9043 100644 > --- a/drivers/staging/lustre/lustre/llite/dir.c > +++ b/drivers/staging/lustre/lustre/llite/dir.c > @@ -1213,29 +1213,31 @@ out: > return rc; > } > > -static char * > -ll_getname(const char __user *filename) > +/* This function tries to get a single name component, > + * to send to the server. No actual path traversal involved, > + * so we limit to NAME_MAX */ > +static char *ll_getname(const char __user *filename) > { > int ret = 0, len; > - char *tmp = __getname(); > + char *tmp = kzalloc(NAME_MAX + 1, GFP_KERNEL); Doing allocations in the declaration block is rare in the kernel but it accounts for around a quarter of the missing NULL checks and many memory leaks in the kbuild zero day bot testing. It's a bad idea and some subsystems ban the practice, but Greg is fine with it so I'm not going to complain. This is me keeping totally silent like a mouse. :P > > if (!tmp) > return ERR_PTR(-ENOMEM); > > - len = strncpy_from_user(tmp, filename, PATH_MAX); > - if (len == 0) > + len = strncpy_from_user(tmp, filename, NAME_MAX); > + if (len < 0) > + ret = len; > + else if (len == 0) > ret = -ENOENT; > - else if (len > PATH_MAX) > - ret = -ENAMETOOLONG; I don't like how this does silent truncation. strncpy_from_user() return -EFAULT if we run into unmapped memory. Otherwise if the user supplies a too long name it returns len == PATH_MAX. (I think, the documentation for this function is hard to understand). Of course, the check was never true in the original code... regards, dan carpenter _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
