[PATCH v2] inotify: invalid mask should return a error number but not set it

[PATCH v2] inotify: invalid mask should return a error number but not set it

[Zhao Hongjiang]

When we run the crackerjack testsuit, inotify_add_watch test is stalled cause the
invalid mask 0, the task is waiting for the event but it never come. This should
return -EINVAL and it do is before the commit 676a0675cf9200 ("inotify: remove 
broken mask checks causing unmount to be EINVAL"). The commit remove the invalid 
mask check simply, but the invalid mask check is needed indeed.

Check the mask wether in the ALL_INOTIFY_BITS before the inotify_arg_to_mask call,
if is not, just return -EINVAL. 

Because IN_UNMOUNT is in ALL_INOTIFY_BITS, so this change will not trigger the 
problem that above commit fixed.

v2: move the check at the begain of inotify_add_watch syscall.

Signed-off-by: Zhao Hongjiang <zhaohongjiang@huawei.com>
Cc: stable@vger.kernel.org
---
 fs/notify/inotify/inotify_user.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index e0f7c12..a8ce1b6 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -572,7 +572,6 @@ static int inotify_update_existing_watch(struct fsnotify_group *group,
 	int add = (arg & IN_MASK_ADD);
 	int ret;
 
-	/* don't allow invalid bits: we don't want flags set */
 	mask = inotify_arg_to_mask(arg);
 
 	fsn_mark = fsnotify_find_inode_mark(group, inode);
@@ -623,7 +622,6 @@ static int inotify_new_watch(struct fsnotify_group *group,
 	struct idr *idr = &group->inotify_data.idr;
 	spinlock_t *idr_lock = &group->inotify_data.idr_lock;
 
-	/* don't allow invalid bits: we don't want flags set */
 	mask = inotify_arg_to_mask(arg);
 
 	tmp_i_mark = kmem_cache_alloc(inotify_inode_mark_cachep, GFP_KERNEL);
@@ -751,6 +749,10 @@ SYSCALL_DEFINE3(inotify_add_watch, int, fd, const char __user *, pathname,
 	int ret;
 	unsigned flags = 0;
 
+	/* don't allow invalid bits: we don't want flags set */
+	if (unlikely(!(arg & ALL_INOTIFY_BITS)))
+		return -EINVAL;
+
 	f = fdget(fd);
 	if (unlikely(!f.file))
 		return -EBADF;
-- 
1.7.1

Re: [PATCH v2] inotify: invalid mask should return a error number but not set it

[Jim Somerville]

This looks good to me, doing the check once right up at the syscall is 
the right place to do it as Paul G pointed out earlier.  Letting bad 
args flow into the bowels of the code leads to the obfuscation like what 
was in that original broken mask check that I removed.  The commit 
header English could use some cleanup though.

-Jim


On 13-04-25 02:51 AM, Zhao Hongjiang wrote:
> When we run the crackerjack testsuit, inotify_add_watch test is stalled cause the
> invalid mask 0, the task is waiting for the event but it never come. This should
> return -EINVAL and it do is before the commit 676a0675cf9200 ("inotify: remove
> broken mask checks causing unmount to be EINVAL"). The commit remove the invalid
> mask check simply, but the invalid mask check is needed indeed.
>
> Check the mask wether in the ALL_INOTIFY_BITS before the inotify_arg_to_mask call,
> if is not, just return -EINVAL.
>
> Because IN_UNMOUNT is in ALL_INOTIFY_BITS, so this change will not trigger the
> problem that above commit fixed.
>
> v2: move the check at the begain of inotify_add_watch syscall.
>
> Signed-off-by: Zhao Hongjiang <zhaohongjiang@huawei.com>
> Cc: stable@vger.kernel.org
> ---
>   fs/notify/inotify/inotify_user.c |    6 ++++--
>   1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
> index e0f7c12..a8ce1b6 100644
> --- a/fs/notify/inotify/inotify_user.c
> +++ b/fs/notify/inotify/inotify_user.c
> @@ -572,7 +572,6 @@ static int inotify_update_existing_watch(struct fsnotify_group *group,
>   	int add = (arg & IN_MASK_ADD);
>   	int ret;
>
> -	/* don't allow invalid bits: we don't want flags set */
>   	mask = inotify_arg_to_mask(arg);
>
>   	fsn_mark = fsnotify_find_inode_mark(group, inode);
> @@ -623,7 +622,6 @@ static int inotify_new_watch(struct fsnotify_group *group,
>   	struct idr *idr = &group->inotify_data.idr;
>   	spinlock_t *idr_lock = &group->inotify_data.idr_lock;
>
> -	/* don't allow invalid bits: we don't want flags set */
>   	mask = inotify_arg_to_mask(arg);
>
>   	tmp_i_mark = kmem_cache_alloc(inotify_inode_mark_cachep, GFP_KERNEL);
> @@ -751,6 +749,10 @@ SYSCALL_DEFINE3(inotify_add_watch, int, fd, const char __user *, pathname,
>   	int ret;
>   	unsigned flags = 0;
>
> +	/* don't allow invalid bits: we don't want flags set */
> +	if (unlikely(!(arg & ALL_INOTIFY_BITS)))
> +		return -EINVAL;
> +
>   	f = fdget(fd);
>   	if (unlikely(!f.file))
>   		return -EBADF;
>

Re: [PATCH v2] inotify: invalid mask should return a error number but not set it

[Andrew Morton]

On Thu, 25 Apr 2013 14:51:15 +0800 Zhao Hongjiang <zhaohongjiang@huawei.com> wrote:

> When we run the crackerjack testsuit, inotify_add_watch test is stalled cause the
> invalid mask 0, the task is waiting for the event but it never come. This should
> return -EINVAL and it do is before the commit 676a0675cf9200 ("inotify: remove 
> broken mask checks causing unmount to be EINVAL"). The commit remove the invalid 
> mask check simply, but the invalid mask check is needed indeed.
> 
> Check the mask wether in the ALL_INOTIFY_BITS before the inotify_arg_to_mask call,
> if is not, just return -EINVAL. 
> 
> Because IN_UNMOUNT is in ALL_INOTIFY_BITS, so this change will not trigger the 
> problem that above commit fixed.
> 
> ...
>
> --- a/fs/notify/inotify/inotify_user.c
> +++ b/fs/notify/inotify/inotify_user.c
> @@ -572,7 +572,6 @@ static int inotify_update_existing_watch(struct fsnotify_group *group,
>  	int add = (arg & IN_MASK_ADD);
>  	int ret;
>  
> -	/* don't allow invalid bits: we don't want flags set */
>  	mask = inotify_arg_to_mask(arg);
>  
>  	fsn_mark = fsnotify_find_inode_mark(group, inode);
> @@ -623,7 +622,6 @@ static int inotify_new_watch(struct fsnotify_group *group,
>  	struct idr *idr = &group->inotify_data.idr;
>  	spinlock_t *idr_lock = &group->inotify_data.idr_lock;
>  
> -	/* don't allow invalid bits: we don't want flags set */
>  	mask = inotify_arg_to_mask(arg);
>  
>  	tmp_i_mark = kmem_cache_alloc(inotify_inode_mark_cachep, GFP_KERNEL);
> @@ -751,6 +749,10 @@ SYSCALL_DEFINE3(inotify_add_watch, int, fd, const char __user *, pathname,
>  	int ret;
>  	unsigned flags = 0;
>  
> +	/* don't allow invalid bits: we don't want flags set */
> +	if (unlikely(!(arg & ALL_INOTIFY_BITS)))
> +		return -EINVAL;
> +
>  	f = fdget(fd);
>  	if (unlikely(!f.file))
>  		return -EBADF;

Doesn't compile and clearly wasn't runtime tested.

--- a/fs/notify/inotify/inotify_user.c~inotify-invalid-mask-should-return-a-error-number-but-not-set-it-fix
+++ a/fs/notify/inotify/inotify_user.c
@@ -750,7 +750,7 @@ SYSCALL_DEFINE3(inotify_add_watch, int,
 	unsigned flags = 0;
 
 	/* don't allow invalid bits: we don't want flags set */
-	if (unlikely(!(arg & ALL_INOTIFY_BITS)))
+	if (unlikely(!(mask & ALL_INOTIFY_BITS)))
 		return -EINVAL;
 
 	f = fdget(fd);
_

Please confirm that this fixed patch has been runtime tested.

btw, calling a function argument "arg" is plain dumb.  That's not a
name of anything.  Someone please go through the file and rename this
to something meaningful.  "mask", at a minimum.