diff mbox series

[v7,1/4] fanotify: return only user requested event types in event mask

Message ID 812e19281cfb4de116fcb8baff1fcddcd63ceb4d.1541639254.git.mbobrowski@mbobrowski.org (mailing list archive)
State New, archived
Headers show
Series fanotify: introduce new event mask FAN_OPEN_EXEC and FAN_OPEN_EXEC_PERM | expand

Commit Message

Matthew Bobrowski Nov. 8, 2018, 3:05 a.m. UTC 
Modify fanotify_should_send_event() so that it now returns a mask for
an event that contains ONLY flags for the event types that have been
specifically requested by the user. Flags that may have been included
within the event mask, but have not been explicitly requested by the
user will not be present in the returned value.

As an example, given the situation where a user requests events of type
FAN_OPEN. Traditionally, the event mask returned within an event that
occurred on a filesystem object that has been marked for monitoring and is
opened, will only ever have the FAN_OPEN bit set. With the introduction of
the new flags like FAN_OPEN_EXEC, and perhaps any other future event
flags, there is a possibility of the returned event mask containing more
than a single bit set, despite having only requested the single event type.
Prior to these modifications performed to fanotify_should_send_event(), a
user would have received a bundled event mask containing flags FAN_OPEN
and FAN_OPEN_EXEC in the instance that a file was opened for execution via
execve(), for example. This means that a user would receive event types
in the returned event mask that have not been requested. This runs the
possibility of breaking existing systems and causing other unforeseen
issues.

To mitigate this possibility, fanotify_should_send_event() has been
modified to return the event mask containing ONLY event types explicitly
requested by the user. This means that we will NOT report events that the
user did no set a mask for, and we will NOT report events that the user
has set an ignore mask for.

The function name fanotify_should_send_event() has also been updated so
that it's more relevant to what it has been designed to do.

Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

Comments

Jan Kara Nov. 13, 2018, 5:38 p.m. UTC | #1 
On Thu 08-11-18 14:05:49, Matthew Bobrowski wrote:
> @@ -131,11 +137,7 @@ static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info,
>  	    !(marks_mask & FS_ISDIR & ~marks_ignored_mask))
>  		return false;

Looking into this before merge, this hunk has apparently slipped during
some rebase (missing false->0 conversion).

> -	if (event_mask & FANOTIFY_OUTGOING_EVENTS &
> -	    marks_mask & ~marks_ignored_mask)
> -		return true;
> -
> -	return false;
> +	return event_mask & FANOTIFY_OUTGOING_EVENTS & marks_mask;

And here we miss & ~marks_ignored_mask, right?

I've changed both in the patch I've merged but I'm checking just to be
sure...

								Honza
Amir Goldstein Nov. 13, 2018, 5:53 p.m. UTC | #2 
On Tue, Nov 13, 2018 at 7:38 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 08-11-18 14:05:49, Matthew Bobrowski wrote:
> > @@ -131,11 +137,7 @@ static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info,
> >           !(marks_mask & FS_ISDIR & ~marks_ignored_mask))
> >               return false;
>
> Looking into this before merge, this hunk has apparently slipped during
> some rebase (missing false->0 conversion).
>
> > -     if (event_mask & FANOTIFY_OUTGOING_EVENTS &
> > -         marks_mask & ~marks_ignored_mask)
> > -             return true;
> > -
> > -     return false;
> > +     return event_mask & FANOTIFY_OUTGOING_EVENTS & marks_mask;
>
> And here we miss & ~marks_ignored_mask, right?
>
> I've changed both in the patch I've merged but I'm checking just to be
> sure...
>
FWIW, changes look correct to me.

Thanks,
Amir.
Matthew Bobrowski Nov. 13, 2018, 11:54 p.m. UTC | #3 
On Tue, Nov 13, 2018 at 07:53:07PM +0200, Amir Goldstein wrote:
> > > @@ -131,11 +137,7 @@ static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info,
> > >           !(marks_mask & FS_ISDIR & ~marks_ignored_mask))
> > >               return false;
> >
> > Looking into this before merge, this hunk has apparently slipped during
> > some rebase (missing false->0 conversion).
> >
> > > -     if (event_mask & FANOTIFY_OUTGOING_EVENTS &
> > > -         marks_mask & ~marks_ignored_mask)
> > > -             return true;
> > > -
> > > -     return false;
> > > +     return event_mask & FANOTIFY_OUTGOING_EVENTS & marks_mask;
> >
> > And here we miss & ~marks_ignored_mask, right?
> >
> > I've changed both in the patch I've merged but I'm checking just to be
> > sure...
> >
> FWIW, changes look correct to me.

Yes, that would be right Jan.
Jan Kara Nov. 14, 2018, 12:04 p.m. UTC | #4 
On Wed 14-11-18 10:54:46, Matthew Bobrowski wrote:
> On Tue, Nov 13, 2018 at 07:53:07PM +0200, Amir Goldstein wrote:
> > > > @@ -131,11 +137,7 @@ static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info,
> > > >           !(marks_mask & FS_ISDIR & ~marks_ignored_mask))
> > > >               return false;
> > >
> > > Looking into this before merge, this hunk has apparently slipped during
> > > some rebase (missing false->0 conversion).
> > >
> > > > -     if (event_mask & FANOTIFY_OUTGOING_EVENTS &
> > > > -         marks_mask & ~marks_ignored_mask)
> > > > -             return true;
> > > > -
> > > > -     return false;
> > > > +     return event_mask & FANOTIFY_OUTGOING_EVENTS & marks_mask;
> > >
> > > And here we miss & ~marks_ignored_mask, right?
> > >
> > > I've changed both in the patch I've merged but I'm checking just to be
> > > sure...
> > >
> > FWIW, changes look correct to me.
> 
> Yes, that would be right Jan. 

Thanks for confirmation. Patches pushed out. Thanks for your work and also
to Amir for his review and comments!

								Honza
diff mbox series

Patch

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index e08a6647267b..0a09950317dd 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -89,7 +89,13 @@  static int fanotify_get_response(struct fsnotify_group *group,
 	return ret;
 }
 
-static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info,
+/*
+ * This function returns a mask for an event that only contains the flags
+ * that have been specifically requested by the user. Flags that may have
+ * been included within the event mask, but have not been explicitly
+ * requested by the user, will not be present in the returned mask.
+ */
+static u32 fanotify_group_event_mask(struct fsnotify_iter_info *iter_info,
 				       u32 event_mask, const void *data,
 				       int data_type)
 {
@@ -101,14 +107,14 @@  static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info,
 	pr_debug("%s: report_mask=%x mask=%x data=%p data_type=%d\n",
 		 __func__, iter_info->report_mask, event_mask, data, data_type);
 
-	/* if we don't have enough info to send an event to userspace say no */
+	/* If we don't have enough info to send an event to userspace say no */
 	if (data_type != FSNOTIFY_EVENT_PATH)
-		return false;
+		return 0;
 
-	/* sorry, fanotify only gives a damn about files and dirs */
+	/* Sorry, fanotify only gives a damn about files and dirs */
 	if (!d_is_reg(path->dentry) &&
 	    !d_can_lookup(path->dentry))
-		return false;
+		return 0;
 
 	fsnotify_foreach_obj_type(type) {
 		if (!fsnotify_iter_should_report_type(iter_info, type))
@@ -131,11 +137,7 @@  static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info,
 	    !(marks_mask & FS_ISDIR & ~marks_ignored_mask))
 		return false;
 
-	if (event_mask & FANOTIFY_OUTGOING_EVENTS &
-	    marks_mask & ~marks_ignored_mask)
-		return true;
-
-	return false;
+	return event_mask & FANOTIFY_OUTGOING_EVENTS & marks_mask;
 }
 
 struct fanotify_event_info *fanotify_alloc_event(struct fsnotify_group *group,
@@ -210,7 +212,8 @@  static int fanotify_handle_event(struct fsnotify_group *group,
 
 	BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 10);
 
-	if (!fanotify_should_send_event(iter_info, mask, data, data_type))
+	mask = fanotify_group_event_mask(iter_info, mask, data, data_type);
+	if (!mask)
 		return 0;
 
 	pr_debug("%s: group=%p inode=%p mask=%x\n", __func__, group, inode,