[PATCH 1/2] fsnotify: fix NULL dereference in send_to_group()

[PATCH 1/2] fsnotify: fix NULL dereference in send_to_group()

[Csaba Henk]

If fanotify is triggered via a vfsmount mark (so that there is
no inode mark, group in send_to_group() is set from a structure
member where the struct pointer is NULL.

This can be tested with the fanotify utility available from
http://people.redhat.com/eparis/fanotify/:

 # fanotify -m / & touch /x

Signed-off-by: Csaba Henk <csaba@gluster.com>
---
 fs/notify/fsnotify.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 3970392..6657315 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -148,13 +148,21 @@ static int send_to_group(struct inode *to_tell, struct vfsmount *mnt,
 			 const unsigned char *file_name,
 			 struct fsnotify_event **event)
 {
-	struct fsnotify_group *group = inode_mark->group;
+	struct fsnotify_group *group;
 	__u32 inode_test_mask = (mask & ~FS_EVENT_ON_CHILD);
 	__u32 vfsmount_test_mask = (mask & ~FS_EVENT_ON_CHILD);
 
-	pr_debug("%s: group=%p to_tell=%p mnt=%p mark=%p mask=%x data=%p"
-		 " data_is=%d cookie=%d event=%p\n", __func__, group, to_tell,
-		 mnt, inode_mark, mask, data, data_is, cookie, *event);
+	if (inode_mark)
+		group = inode_mark->group;
+	else if (vfsmount_mark)
+		group = vfsmount_mark->group;
+	else
+		BUG();
+
+	pr_debug("%s: group=%p to_tell=%p mnt=%p inode_mark=%p vfsmount_mark=%p"
+		 " mask=%x data=%p data_is=%d cookie=%d event=%p\n",
+		 __func__, group, to_tell, mnt, inode_mark, vfsmount_mark, mask,
+		 data, data_is, cookie, *event);
 
 	/* clear ignored on inode modification */
 	if (mask & FS_MODIFY) {
-- 
1.7.2.2

[PATCH 2/2] fsnotify: refactor mask filtering in send_to_group()

[Csaba Henk]

- isolate filtering logic to reduce code duplication
- the case when only one mark is passed was not handled correctly:
  in this case the mask variable for the other mark was not
  filtered, giving therefore a green way, regardless the mask of
  the mark which was actually there to be tested

Signed-off-by: Csaba Henk <csaba@gluster.com>
---
 fs/notify/fsnotify.c |   44 +++++++++++++++++---------------------------
 1 files changed, 17 insertions(+), 27 deletions(-)

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 6657315..ece4bd6 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -140,6 +140,20 @@ void __fsnotify_parent(struct path *path, struct dentry *dentry, __u32 mask)
 }
 EXPORT_SYMBOL_GPL(__fsnotify_parent);
 
+static int check_mark_mask(struct fsnotify_mark *mark, __u32 *mask)
+{
+	if (!mark)
+		return 0;
+
+	if ((*mask & FS_MODIFY) &&
+	    !(mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY))
+		mark->ignored_mask = 0;
+	else
+		*mask &= ~mark->ignored_mask;
+
+	return *mask & mark->mask;
+}
+
 static int send_to_group(struct inode *to_tell, struct vfsmount *mnt,
 			 struct fsnotify_mark *inode_mark,
 			 struct fsnotify_mark *vfsmount_mark,
@@ -149,8 +163,7 @@ static int send_to_group(struct inode *to_tell, struct vfsmount *mnt,
 			 struct fsnotify_event **event)
 {
 	struct fsnotify_group *group;
-	__u32 inode_test_mask = (mask & ~FS_EVENT_ON_CHILD);
-	__u32 vfsmount_test_mask = (mask & ~FS_EVENT_ON_CHILD);
+	__u32 test_mask = (mask & ~FS_EVENT_ON_CHILD);
 
 	if (inode_mark)
 		group = inode_mark->group;
@@ -164,31 +177,8 @@ static int send_to_group(struct inode *to_tell, struct vfsmount *mnt,
 		 __func__, group, to_tell, mnt, inode_mark, vfsmount_mark, mask,
 		 data, data_is, cookie, *event);
 
-	/* clear ignored on inode modification */
-	if (mask & FS_MODIFY) {
-		if (inode_mark &&
-		    !(inode_mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY))
-			inode_mark->ignored_mask = 0;
-		if (vfsmount_mark &&
-		    !(vfsmount_mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY))
-			vfsmount_mark->ignored_mask = 0;
-	}
-
-	/* does the inode mark tell us to do something? */
-	if (inode_mark) {
-		inode_test_mask &= inode_mark->mask;
-		inode_test_mask &= ~inode_mark->ignored_mask;
-	}
-
-	/* does the vfsmount_mark tell us to do something? */
-	if (vfsmount_mark) {
-		vfsmount_test_mask &= vfsmount_mark->mask;
-		vfsmount_test_mask &= ~vfsmount_mark->ignored_mask;
-		if (inode_mark)
-			vfsmount_test_mask &= ~inode_mark->ignored_mask;
-	}
-
-	if (!inode_test_mask && !vfsmount_test_mask)
+	if (!(check_mark_mask(inode_mark, &test_mask) ||
+	      check_mark_mask(vfsmount_mark, &test_mask)))
 		return 0;
 
 	if (group->ops->should_send_event(group, to_tell, inode_mark,
-- 
1.7.2.2

Re: [PATCH 1/2] fsnotify: fix NULL dereference in send_to_group()

[Eric Paris]

On Sun, 2010-08-29 at 00:55 +0530, Csaba Henk wrote:
> If fanotify is triggered via a vfsmount mark (so that there is
> no inode mark, group in send_to_group() is set from a structure
> member where the struct pointer is NULL.
> 
> This can be tested with the fanotify utility available from
> http://people.redhat.com/eparis/fanotify/:
> 
>  # fanotify -m / & touch /x

This should be fixed in the pull request I sent to Linus last night.
Sorry you had to track it down as well.  There are a number of other bug
fixes in my tree

http://git.infradead.org/users/eparis/notify.git

There might still be some code duplication which something like 2/2
could clean up but your patch does apply to my devel tree and it has a
logic flaw.  In the case we have both a vfsmount and an inode mark we
need to test:

event_mask & vmark->mask & ~vmark->ignored_mask & ~imark->ignored mask.

You would only ever test one or the other, not both together like that.

Thanks!  Please let me know any other problems you run into!

-Eric

Re: [PATCH 1/2] fsnotify: fix NULL dereference in send_to_group()

[Csaba Henk]

On Sun, Aug 29, 2010 at 2:49 AM, Eric Paris <eparis@redhat.com> wrote:
> This should be fixed in the pull request I sent to Linus last night.
> Sorry you had to track it down as well.  There are a number of other bug
> fixes in my tree
>
> http://git.infradead.org/users/eparis/notify.git

Oh yeah, my bad, I did check that tree before getting into this... but
only the master branch, not for-linus.

> There might still be some code duplication which something like 2/2
> could clean up but your patch does apply to my devel tree and it has a
> logic flaw.  In the case we have both a vfsmount and an inode mark we
> need to test:
>
> event_mask & vmark->mask & ~vmark->ignored_mask & ~imark->ignored mask.
>
> You would only ever test one or the other, not both together like that.

Indeed my code implements that behavior. I took care to pass the
test_mask by reference to check_mark_mask(), where upon the first
invocation it's reduced with ~imark->ignored mask, so for the second
invocation with the vmark we make test as the above expression.

If you are OK with the clean-up, I can readjust it to apply.

Csaba