Fixup lusre ll_getname
cover Fixup lusre ll_getname
From: Oleg Drokin <[email protected]>
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(-)
Signature · 1 line
2.1.0
PATCH 1/2 staging/lustre/llite: remove LL_IOC_REMOVE_ENTRY handler
From: Oleg Drokin <[email protected]>
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.
--- drivers/staging/lustre/lustre/llite/dir.c | 29 ----------------------------- 1 file changed, 29 deletions(-)
drivers/staging/lustre/lustre/llite/dir.c +0 -29
@@ -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 :
Signature · 1 line
2.1.0
Re: [PATCH 1/2] staging/lustre/llite: remove LL_IOC_REMOVE_ENTRY handler
Quoted 5 lines · On Wed, Jun 10, 2015 at 12:41:22AM -0400, [email protected] wrote:
From: Oleg Drokin <[email protected]>
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
Quoted 8 lines · Sudip Mukherjee
Quoted 5 lines · On Wed, Jun 10, 2015 at 12:41:22AM -0400, [email protected] wrote:
From: Oleg Drokin <[email protected]>
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).
Quoted 2 lines
And I think you missed removing the #define
LL_IOC_REMOVE_ENTRYfromlustre_user.hwhich will not be used anymore.
Good point.
Thanks.
Bye,
Oleg
PATCH 2/2 staging/lustre/llite: fix ll_getname user buffer copy
In reply to [email protected]From: Oleg Drokin <[email protected]>
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
--- drivers/staging/lustre/lustre/llite/dir.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)
drivers/staging/lustre/lustre/llite/dir.c +11 -9
@@ -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) {
Signature · 1 line
2.1.0
Re: [PATCH 2/2] staging/lustre/llite: fix ll_getname user buffer copy
Quoted 24 lines · On Wed, Jun 10, 2015 at 12:41:23AM -0400, [email protected] wrote:
From: Oleg Drokin <[email protected]>
strncpy_from_usercould return negative values on error,
so need to take those into account.
Sincell_getnameis 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 <[email protected]>--- drivers/staging/lustre/lustre/llite/dir.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)drivers/staging/lustre/lustre/llite/dir.c +0
-0@@ -1213,29 +1213,31 @@ out:return rc; }
drivers/staging/lustre/lustre/llite/dir.c out: · line 1222
Quoted 10 lines
drivers/staging/lustre/lustre/llite/dir.c +5
-3-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
drivers/staging/lustre/lustre/llite/dir.c out: · line 1229
Quoted 13 lines
drivers/staging/lustre/lustre/llite/dir.c +4
-4if (!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
Quoted 25 lines · Dan Carpenter
Quoted 24 lines · On Wed, Jun 10, 2015 at 12:41:23AM -0400, [email protected] wrote:
From: Oleg Drokin <[email protected]>
strncpy_from_usercould return negative values on error,
so need to take those into account.
Sincell_getnameis 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 <[email protected]>---
drivers/staging/lustre/lustre/llite/dir.c| 20 +++++++++++---------1 file changed, 11 insertions(+), 9 deletions(-)drivers/staging/lustre/lustre/llite/dir.c +0
-0@@ -1213,29 +1213,31 @@ out:return rc; }
Quoted 16 lines
drivers/staging/lustre/lustre/llite/dir.c out: · line 1222
Quoted 10 lines
drivers/staging/lustre/lustre/llite/dir.c +5
-3-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.
Quoted 19 lines
drivers/staging/lustre/lustre/llite/dir.c out: · line 1229
Quoted 13 lines
drivers/staging/lustre/lustre/llite/dir.c +4
-4if (!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-EFAULTif 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).
Quoted 1 line
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