Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754192AbbFJHwm (ORCPT ); Wed, 10 Jun 2015 03:52:42 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:36186 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933319AbbFJHw1 (ORCPT ); Wed, 10 Jun 2015 03:52:27 -0400 Date: Wed, 10 Jun 2015 10:52:09 +0300 From: Dan Carpenter To: green@linuxhacker.ru Cc: Greg Kroah-Hartman , devel@driverdev.osuosl.org, Andreas Dilger , Linux Kernel Mailing List Subject: Re: [PATCH 2/2] staging/lustre/llite: fix ll_getname user buffer copy Message-ID: <20150610075209.GC28762@mwanda> References: <1433911283-24902-1-git-send-email-green@linuxhacker.ru> <1433911283-24902-3-git-send-email-green@linuxhacker.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1433911283-24902-3-git-send-email-green@linuxhacker.ru> User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: userv0021.oracle.com [156.151.31.71] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2670 Lines: 73 On Wed, Jun 10, 2015 at 12:41:23AM -0400, green@linuxhacker.ru wrote: > From: Oleg Drokin > > 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 > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/