[PATCH 0/2] Fixup lusre ll_getname

[PATCH 0/2] Fixup lusre ll_getname

[green]

From: Oleg Drokin <green@linuxhacker.ru>

Some time ago Al Viro noticed that lustre ll_getname is broken.
At the time a patch was submitted to convert lustre to use
exported getname, that was rejected by hch on the grounds that
filesystem code sould not really be reimplementing their own
lookups which kind of made sense back then.

But upon further investigation it seems that ll_getname is
used in a different way, it only gets a single path name
component that is then shiped to the server for some operations.
Going through VFS here to do proper lookups is not really
all that good of an idea since dcache pollution is undesired
at the very least.

So these two patches drop one of the ll_getname users that
could be done in another way and fix up ll_getname to only
allocate a single pathname component buffer and properly check
copy from userspace return value.

Please consider.

Oleg Drokin (2):
  staging/lustre/llite: remove LL_IOC_REMOVE_ENTRY handler
  staging/lustre/llite: fix ll_getname

 drivers/staging/lustre/lustre/llite/dir.c | 49 +++++++------------------------
 1 file changed, 11 insertions(+), 38 deletions(-)

-- 
2.1.0

[PATCH 1/2] staging/lustre/llite: remove LL_IOC_REMOVE_ENTRY handler

[green]

From: Oleg Drokin <green@linuxhacker.ru>

It uses getname in unsafe manner and since it's to deal with corrupted
or inconsistent filesystem, we are probably better to deal with
it from lfsck anyway.

Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
---
 drivers/staging/lustre/lustre/llite/dir.c | 29 -----------------------------
 1 file changed, 29 deletions(-)

diff --git a/drivers/staging/lustre/lustre/llite/dir.c b/drivers/staging/lustre/lustre/llite/dir.c
index 4b0de8d..87a042c 100644
--- a/drivers/staging/lustre/lustre/llite/dir.c
+++ b/drivers/staging/lustre/lustre/llite/dir.c
@@ -1436,35 +1436,6 @@ free_lmv:
 		kfree(tmp);
 		return rc;
 	}
-	case LL_IOC_REMOVE_ENTRY: {
-		char		*filename = NULL;
-		int		 namelen = 0;
-		int		 rc;
-
-		/* Here is a little hack to avoid sending REINT_RMENTRY to
-		 * unsupported server, which might crash the server(LU-2730),
-		 * Because both LVB_TYPE and REINT_RMENTRY will be supported
-		 * on 2.4, we use OBD_CONNECT_LVB_TYPE to detect whether the
-		 * server will support REINT_RMENTRY XXX*/
-		if (!(exp_connect_flags(sbi->ll_md_exp) & OBD_CONNECT_LVB_TYPE))
-			return -ENOTSUPP;
-
-		filename = ll_getname((const char *)arg);
-		if (IS_ERR(filename))
-			return PTR_ERR(filename);
-
-		namelen = strlen(filename);
-		if (namelen < 1) {
-			rc = -EINVAL;
-			goto out_rmdir;
-		}
-
-		rc = ll_rmdir_entry(inode, filename, namelen);
-out_rmdir:
-		if (filename)
-			ll_putname(filename);
-		return rc;
-	}
 	case LL_IOC_LOV_SWAP_LAYOUTS:
 		return -EPERM;
 	case LL_IOC_OBD_STATFS:
-- 
2.1.0

[PATCH 2/2] staging/lustre/llite: fix ll_getname user buffer copy

[green]

From: Oleg Drokin <green@linuxhacker.ru>

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@linuxhacker.ru>
---
 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);
 
 	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;
 
 	if (ret) {
-		__putname(tmp);
+		kfree(tmp);
 		tmp =  ERR_PTR(ret);
 	}
 	return tmp;
 }
 
-#define ll_putname(filename) __putname(filename)
+#define ll_putname(filename) kfree(filename)
 
 static long ll_dir_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
-- 
2.1.0

Re: [PATCH 1/2] staging/lustre/llite: remove LL_IOC_REMOVE_ENTRY handler

[Sudip Mukherjee]

On Wed, Jun 10, 2015 at 12:41:22AM -0400, green@linuxhacker.ru wrote:
> From: Oleg Drokin <green@linuxhacker.ru>
> 
> It uses getname in unsafe manner and since it's to deal with corrupted
> or inconsistent filesystem, we are probably better to deal with
> it from lfsck anyway.
I am not sure but will it not break the userspace? There may be some
application who is using this ioctl.
And I think you missed removing the #define LL_IOC_REMOVE_ENTRY from
lustre_user.h which will not be used anymore. 

regards
sudip

Re: [PATCH 1/2] staging/lustre/llite: remove LL_IOC_REMOVE_ENTRY handler

[Oleg Drokin]

On Jun 10, 2015, at 3:08 AM, Sudip Mukherjee wrote:

> On Wed, Jun 10, 2015 at 12:41:22AM -0400, green@linuxhacker.ru wrote:
>> From: Oleg Drokin <green@linuxhacker.ru>
>> 
>> It uses getname in unsafe manner and since it's to deal with corrupted
>> or inconsistent filesystem, we are probably better to deal with
>> it from lfsck anyway.
> I am not sure but will it not break the userspace? There may be some
> application who is using this ioctl.

This is an ioctl for a relatively new "distributed namespace" feature.
Our lustre control tool uses it in order to get rid of presumed
incorrect entries that don't point anywhere. But in fact that's pretty
risky and I am of the opinion this is best left to fsck to decide,
it's too easy to create nameless directories otherwise
(and even if we only allow sysadmin to do this, it's still not all that
great of an idea).

> And I think you missed removing the #define LL_IOC_REMOVE_ENTRY from
> lustre_user.h which will not be used anymore. 

Good point.
Thanks.

Bye,
    Oleg

Re: [PATCH 2/2] staging/lustre/llite: fix ll_getname user buffer copy

[Dan Carpenter]

On Wed, Jun 10, 2015 at 12:41:23AM -0400, green@linuxhacker.ru wrote:
> From: Oleg Drokin <green@linuxhacker.ru>
> 
> 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@linuxhacker.ru>
> ---
>  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

Re: [PATCH 2/2] staging/lustre/llite: fix ll_getname user buffer copy

[Oleg Drokin]

On Jun 10, 2015, at 3:52 AM, Dan Carpenter wrote:

> On Wed, Jun 10, 2015 at 12:41:23AM -0400, green@linuxhacker.ru wrote:
>> From: Oleg Drokin <green@linuxhacker.ru>
>> 
>> 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@linuxhacker.ru>
>> ---
>> 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.

Fair. I can redo this.

>> 
>> 	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…

Right.
It's no big deal to ask for NAME_MAX+1 and then restore the len > NAME_MAX
check and return -ENAMETOOLONG in that case. Then the silent truncate
is gone and logic wise we don't care all that much either way, I imagine.

Bye,
    Oleg