[PATCH] inotify: actually check for invalid bits in sys_inotify_add_watch()

[PATCH] inotify: actually check for invalid bits in sys_inotify_add_watch()

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

The comment here says that it is checking for invalid bits.  But,
the mask is *actually* checking to ensure that _any_ valid bit
is set, which is quite different.

Add the actual check which was intended.  Retain the existing
check because it actually does something useful: ensure that some
inotify bits are being added to the watch.  Plus, this is
existing behavior which would be nice to preserve.

I did a quick sniff test that inotify functions and that my
'inotify-tools' package passes 'make check'.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: John McCutchan <john@johnmccutchan.com> (maintainer:INOTIFY)
Cc: Robert Love <rlove@rlove.org> (maintainer:INOTIFY)
Cc: Eric Paris <eparis@parisplace.org> (maintainer:INOTIFY)
Cc: linux-kernel@vger.kernel.org (open list)
Cc: stable@vger.kernel.org
---

 b/fs/notify/inotify/inotify_user.c |    3 +++
 1 file changed, 3 insertions(+)

diff -puN fs/notify/inotify/inotify_user.c~inotify-EINVAL-on-invalid-bit fs/notify/inotify/inotify_user.c
--- a/fs/notify/inotify/inotify_user.c~inotify-EINVAL-on-invalid-bit	2015-06-26 13:33:30.277219285 -0700
+++ b/fs/notify/inotify/inotify_user.c	2015-06-26 13:35:19.026122033 -0700
@@ -707,6 +707,9 @@ SYSCALL_DEFINE3(inotify_add_watch, int,
 	unsigned flags = 0;
 
 	/* don't allow invalid bits: we don't want flags set */
+	if (unlikely(mask & ~ALL_INOTIFY_BITS))
+		return -EINVAL;
+	/* require at least one valid bit set in the mask */
 	if (unlikely(!(mask & ALL_INOTIFY_BITS)))
 		return -EINVAL;
 
_

Re: [PATCH] inotify: actually check for invalid bits in sys_inotify_add_watch()

[Dave Hansen]

On 06/30/2015 10:36 AM, Dave Hansen wrote:
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> The comment here says that it is checking for invalid bits.  But,
> the mask is *actually* checking to ensure that _any_ valid bit
> is set, which is quite different.
> 
> Add the actual check which was intended.  Retain the existing
> check because it actually does something useful: ensure that some
> inotify bits are being added to the watch.  Plus, this is
> existing behavior which would be nice to preserve.
> 
> I did a quick sniff test that inotify functions and that my
> 'inotify-tools' package passes 'make check'.

Did anybody have any comments on this patch?  Who picks up inotify patches?

>  b/fs/notify/inotify/inotify_user.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff -puN fs/notify/inotify/inotify_user.c~inotify-EINVAL-on-invalid-bit fs/notify/inotify/inotify_user.c
> --- a/fs/notify/inotify/inotify_user.c~inotify-EINVAL-on-invalid-bit	2015-06-26 13:33:30.277219285 -0700
> +++ b/fs/notify/inotify/inotify_user.c	2015-06-26 13:35:19.026122033 -0700
> @@ -707,6 +707,9 @@ SYSCALL_DEFINE3(inotify_add_watch, int,
>  	unsigned flags = 0;
>  
>  	/* don't allow invalid bits: we don't want flags set */
> +	if (unlikely(mask & ~ALL_INOTIFY_BITS))
> +		return -EINVAL;
> +	/* require at least one valid bit set in the mask */
>  	if (unlikely(!(mask & ALL_INOTIFY_BITS)))
>  		return -EINVAL;
>  
> _
> 

Re: [PATCH] inotify: actually check for invalid bits in sys_inotify_add_watch()

[Josh Boyer]

On Wed, Sep 9, 2015 at 5:59 PM, Dave Hansen <dave@sr71.net> wrote:
> On 06/30/2015 10:36 AM, Dave Hansen wrote:
>> From: Dave Hansen <dave.hansen@linux.intel.com>
>>
>> The comment here says that it is checking for invalid bits.  But,
>> the mask is *actually* checking to ensure that _any_ valid bit
>> is set, which is quite different.
>>
>> Add the actual check which was intended.  Retain the existing
>> check because it actually does something useful: ensure that some
>> inotify bits are being added to the watch.  Plus, this is
>> existing behavior which would be nice to preserve.
>>
>> I did a quick sniff test that inotify functions and that my
>> 'inotify-tools' package passes 'make check'.
>
> Did anybody have any comments on this patch?  Who picks up inotify patches?

Theoretically the people you have CC'd pick them up.  However it seems
for quite some time Andrew has been shepherding them along.

josh

>>  b/fs/notify/inotify/inotify_user.c |    3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff -puN fs/notify/inotify/inotify_user.c~inotify-EINVAL-on-invalid-bit fs/notify/inotify/inotify_user.c
>> --- a/fs/notify/inotify/inotify_user.c~inotify-EINVAL-on-invalid-bit  2015-06-26 13:33:30.277219285 -0700
>> +++ b/fs/notify/inotify/inotify_user.c        2015-06-26 13:35:19.026122033 -0700
>> @@ -707,6 +707,9 @@ SYSCALL_DEFINE3(inotify_add_watch, int,
>>       unsigned flags = 0;
>>
>>       /* don't allow invalid bits: we don't want flags set */
>> +     if (unlikely(mask & ~ALL_INOTIFY_BITS))
>> +             return -EINVAL;
>> +     /* require at least one valid bit set in the mask */
>>       if (unlikely(!(mask & ALL_INOTIFY_BITS)))
>>               return -EINVAL;
>>
>> _
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe stable" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] inotify: actually check for invalid bits in sys_inotify_add_watch()

[Eric Paris]

Looks fine to me. And usually akpm picks them up these days.

On Wed, 2015-09-09 at 14:59 -0700, Dave Hansen wrote:
> On 06/30/2015 10:36 AM, Dave Hansen wrote:
> > From: Dave Hansen <dave.hansen@linux.intel.com>
> > 
> > The comment here says that it is checking for invalid bits.  But,
> > the mask is *actually* checking to ensure that _any_ valid bit
> > is set, which is quite different.
> > 
> > Add the actual check which was intended.  Retain the existing
> > check because it actually does something useful: ensure that some
> > inotify bits are being added to the watch.  Plus, this is
> > existing behavior which would be nice to preserve.
> > 
> > I did a quick sniff test that inotify functions and that my
> > 'inotify-tools' package passes 'make check'.
> 
> Did anybody have any comments on this patch?  Who picks up inotify
> patches?
> 
> >  b/fs/notify/inotify/inotify_user.c |    3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff -puN fs/notify/inotify/inotify_user.c~inotify-EINVAL-on
> > -invalid-bit fs/notify/inotify/inotify_user.c
> > --- a/fs/notify/inotify/inotify_user.c~inotify-EINVAL-on-invalid
> > -bit	2015-06-26 13:33:30.277219285 -0700
> > +++ b/fs/notify/inotify/inotify_user.c	2015-06-26
> > 13:35:19.026122033 -0700
> > @@ -707,6 +707,9 @@ SYSCALL_DEFINE3(inotify_add_watch, int,
> >  	unsigned flags = 0;
> >  
> >  	/* don't allow invalid bits: we don't want flags set */
> > +	if (unlikely(mask & ~ALL_INOTIFY_BITS))
> > +		return -EINVAL;
> > +	/* require at least one valid bit set in the mask */
> >  	if (unlikely(!(mask & ALL_INOTIFY_BITS)))
> >  		return -EINVAL;
> >  
> > _
> > 
> 

Re: [PATCH] inotify: actually check for invalid bits in sys_inotify_add_watch()

[Dave Hansen]

On 09/09/2015 04:16 PM, Eric Paris wrote:
> Looks fine to me. And usually akpm picks them up these days.

Is that an Acked-by? :)

Re: [PATCH] inotify: actually check for invalid bits in sys_inotify_add_watch()

[Andrew Morton]

On Wed, 9 Sep 2015 16:32:37 -0700 Dave Hansen <dave@sr71.net> wrote:

> On 09/09/2015 04:16 PM, Eric Paris wrote:
> > Looks fine to me. And usually akpm picks them up these days.
> 
> Is that an Acked-by? :)

I grabbed it for 4.3.

I removed your cc:stable.  I don't see anything in here which warrants
a backport.  If there *is* a reason for backporting then your
changelogological skills are sorely wanting!


The changelog is pretty sucky really.  What are the reasons for this
change, apart from "do what the comment said"?  What's the benefit?

And the code comment sucks.  "don't allow invalid bits": well duh.  And
"we don't want flags set" is also useless: it doesn't explain *why* we
don't want those flags set.

And given that there is potential to break existing userspace, we need
some decent reasons for making this change.




From: Dave Hansen <dave.hansen@linux.intel.com>
Subject: inotify: actually check for invalid bits in sys_inotify_add_watch()

The comment here says that it is checking for invalid bits.  But, the mask
is *actually* checking to ensure that _any_ valid bit is set, which is
quite different.

Add the actual check which was intended.  Retain the existing check
because it actually does something useful: ensure that some inotify bits
are being added to the watch.  Plus, this is existing behavior which would
be nice to preserve.

I did a quick sniff test that inotify functions and that my
'inotify-tools' package passes 'make check'.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: John McCutchan <john@johnmccutchan.com>
Cc: Robert Love <rlove@rlove.org>
Cc: Eric Paris <eparis@parisplace.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/notify/inotify/inotify_user.c |    3 +++
 1 file changed, 3 insertions(+)

diff -puN fs/notify/inotify/inotify_user.c~inotify-actually-check-for-invalid-bits-in-sys_inotify_add_watch fs/notify/inotify/inotify_user.c
--- a/fs/notify/inotify/inotify_user.c~inotify-actually-check-for-invalid-bits-in-sys_inotify_add_watch
+++ a/fs/notify/inotify/inotify_user.c
@@ -707,6 +707,9 @@ SYSCALL_DEFINE3(inotify_add_watch, int,
 	unsigned flags = 0;
 
 	/* don't allow invalid bits: we don't want flags set */
+	if (unlikely(mask & ~ALL_INOTIFY_BITS))
+		return -EINVAL;
+	/* require at least one valid bit set in the mask */
 	if (unlikely(!(mask & ALL_INOTIFY_BITS)))
 		return -EINVAL;
 
_

Re: [PATCH] inotify: actually check for invalid bits in sys_inotify_add_watch()

[Andrey Wagin]

2015-09-10 23:49 GMT+03:00 Andrew Morton <akpm@linux-foundation.org>:
> On Wed, 9 Sep 2015 16:32:37 -0700 Dave Hansen <dave@sr71.net> wrote:
>
>> On 09/09/2015 04:16 PM, Eric Paris wrote:
>> > Looks fine to me. And usually akpm picks them up these days.
>>
>> Is that an Acked-by? :)
>
> I grabbed it for 4.3.

This patch breaks CRIU. When we are dumping an inotify file
descriptors, we read event masks from /proc/self/fdinfo.

[root@fc22-vm criu]# cat /proc/1065/fdinfo/3
pos: 0
flags: 04000
mnt_id: 11
inotify wd:1 ino:101d5c sdev:fc00002 mask:800a708 ignored_mask:0
fhandle-bytes:8 fhandle-type:1 f_handle:5c1d10003996cda2

Here the mask contains the 0x8000000 (FS_EVENT_ON_CHILD) flag, which
is set for all inotify and dnotify watchers. On restore, we call
inotify_add_watch() with this mask, and it fails with this patch:

1375  inotify_add_watch(3, "/proc/self/fd/5",
IN_CLOSE_WRITE|IN_CREATE|IN_DELETE|IN_DELETE_SELF|IN_UNMOUNT|IN_IGNORED|0x8000000)
= -1 EINVAL (Invalid argument)

I am not sure that we have to save backward compatibility here. Maybe
we should not show FS_EVENT_ON_CHILD in fdinfo.

>
> I removed your cc:stable.  I don't see anything in here which warrants
> a backport.  If there *is* a reason for backporting then your
> changelogological skills are sorely wanting!
>
>
> The changelog is pretty sucky really.  What are the reasons for this
> change, apart from "do what the comment said"?  What's the benefit?
>
> And the code comment sucks.  "don't allow invalid bits": well duh.  And
> "we don't want flags set" is also useless: it doesn't explain *why* we
> don't want those flags set.
>
> And given that there is potential to break existing userspace, we need
> some decent reasons for making this change.
>
>
>
>
> From: Dave Hansen <dave.hansen@linux.intel.com>
> Subject: inotify: actually check for invalid bits in sys_inotify_add_watch()
>
> The comment here says that it is checking for invalid bits.  But, the mask
> is *actually* checking to ensure that _any_ valid bit is set, which is
> quite different.
>
> Add the actual check which was intended.  Retain the existing check
> because it actually does something useful: ensure that some inotify bits
> are being added to the watch.  Plus, this is existing behavior which would
> be nice to preserve.
>
> I did a quick sniff test that inotify functions and that my
> 'inotify-tools' package passes 'make check'.
>
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: John McCutchan <john@johnmccutchan.com>
> Cc: Robert Love <rlove@rlove.org>
> Cc: Eric Paris <eparis@parisplace.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
>
>  fs/notify/inotify/inotify_user.c |    3 +++
>  1 file changed, 3 insertions(+)
>
> diff -puN fs/notify/inotify/inotify_user.c~inotify-actually-check-for-invalid-bits-in-sys_inotify_add_watch fs/notify/inotify/inotify_user.c
> --- a/fs/notify/inotify/inotify_user.c~inotify-actually-check-for-invalid-bits-in-sys_inotify_add_watch
> +++ a/fs/notify/inotify/inotify_user.c
> @@ -707,6 +707,9 @@ SYSCALL_DEFINE3(inotify_add_watch, int,
>         unsigned flags = 0;
>
>         /* don't allow invalid bits: we don't want flags set */
> +       if (unlikely(mask & ~ALL_INOTIFY_BITS))
> +               return -EINVAL;
> +       /* require at least one valid bit set in the mask */
>         if (unlikely(!(mask & ALL_INOTIFY_BITS)))
>                 return -EINVAL;
>
> _
>
> --
> 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/