fanotify: Disallow permission events for proc filesystem
diff mbox series

Message ID 20190515193337.11167-1-jack@suse.cz
State New
Headers show
Series
  • fanotify: Disallow permission events for proc filesystem
Related show

Commit Message

Jan Kara May 15, 2019, 7:33 p.m. UTC
Proc filesystem has special locking rules for various files. Thus
fanotify which opens files on event delivery can easily deadlock
against another process that waits for fanotify permission event to be
handled. Since permission events on /proc have doubtful value anyway,
just disallow them.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/notify/fanotify/fanotify_user.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Amir Goldstein May 16, 2019, 5:54 a.m. UTC | #1
On Wed, May 15, 2019 at 10:33 PM Jan Kara <jack@suse.cz> wrote:
>
> Proc filesystem has special locking rules for various files. Thus
> fanotify which opens files on event delivery can easily deadlock
> against another process that waits for fanotify permission event to be
> handled. Since permission events on /proc have doubtful value anyway,
> just disallow them.
>

Let's add context:
Link: https://lore.kernel.org/linux-fsdevel/20190320131642.GE9485@quack2.suse.cz/
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/notify/fanotify/fanotify_user.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index a90bb19dcfa2..73719949faa6 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -920,6 +920,20 @@ static int fanotify_test_fid(struct path *path, __kernel_fsid_t *fsid)
>         return 0;
>  }
>
> +static int fanotify_events_supported(struct path *path, __u64 mask)
> +{
> +       /*
> +        * Proc is special and various files have special locking rules so
> +        * fanotify permission events have high chances of deadlocking the
> +        * system. Just disallow them.
> +        */
> +       if (mask & FANOTIFY_PERM_EVENTS &&
> +           !strcmp(path->mnt->mnt_sb->s_type->name, "proc")) {

Better use an SB_I_ flag to forbid permission events on fs?

> +               return -EOPNOTSUPP;

I would go with EINVAL following precedent of per filesystem flags
check on rename(2), but not insisting.

Anyway, following Matthew's man page update for FAN_REPORT_FID,
we should also add this as reason for EOPNOTSUPP/EINVAL.

Thanks,
Amir.
Jan Kara May 16, 2019, 8:36 a.m. UTC | #2
On Thu 16-05-19 08:54:37, Amir Goldstein wrote:
> On Wed, May 15, 2019 at 10:33 PM Jan Kara <jack@suse.cz> wrote:
> >
> > Proc filesystem has special locking rules for various files. Thus
> > fanotify which opens files on event delivery can easily deadlock
> > against another process that waits for fanotify permission event to be
> > handled. Since permission events on /proc have doubtful value anyway,
> > just disallow them.
> >
> 
> Let's add context:
> Link: https://lore.kernel.org/linux-fsdevel/20190320131642.GE9485@quack2.suse.cz/

OK, will add.

> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/notify/fanotify/fanotify_user.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > index a90bb19dcfa2..73719949faa6 100644
> > --- a/fs/notify/fanotify/fanotify_user.c
> > +++ b/fs/notify/fanotify/fanotify_user.c
> > @@ -920,6 +920,20 @@ static int fanotify_test_fid(struct path *path, __kernel_fsid_t *fsid)
> >         return 0;
> >  }
> >
> > +static int fanotify_events_supported(struct path *path, __u64 mask)
> > +{
> > +       /*
> > +        * Proc is special and various files have special locking rules so
> > +        * fanotify permission events have high chances of deadlocking the
> > +        * system. Just disallow them.
> > +        */
> > +       if (mask & FANOTIFY_PERM_EVENTS &&
> > +           !strcmp(path->mnt->mnt_sb->s_type->name, "proc")) {
> 
> Better use an SB_I_ flag to forbid permission events on fs?

So checking s_type->name indeed felt dirty. I don't think we need a
superblock flag though. I'll probably just go with FS_XXX flag in
file_system_type.

> 
> > +               return -EOPNOTSUPP;
> 
> I would go with EINVAL following precedent of per filesystem flags
> check on rename(2), but not insisting.

I was undecided between EOPNOTSUPP and EINVAL. So let's go with EINVAL.

> Anyway, following Matthew's man page update for FAN_REPORT_FID,
> we should also add this as reason for EOPNOTSUPP/EINVAL.

Good point.

Thanks for review!

								Honza
Matthew Bobrowski May 21, 2019, 9:57 p.m. UTC | #3
On Thu, May 16, 2019 at 10:36:32AM +0200, Jan Kara wrote:
> On Thu 16-05-19 08:54:37, Amir Goldstein wrote:
> > On Wed, May 15, 2019 at 10:33 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > Proc filesystem has special locking rules for various files. Thus
> > > fanotify which opens files on event delivery can easily deadlock
> > > against another process that waits for fanotify permission event to be
> > > handled. Since permission events on /proc have doubtful value anyway,
> > > just disallow them.
> > >
> > 
> > Let's add context:
> > Link: https://lore.kernel.org/linux-fsdevel/20190320131642.GE9485@quack2.suse.cz/
> 
> OK, will add.
> 
> > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > ---
> > >  fs/notify/fanotify/fanotify_user.c | 20 ++++++++++++++++++++
> > >  1 file changed, 20 insertions(+)
> > >
> > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > > index a90bb19dcfa2..73719949faa6 100644
> > > --- a/fs/notify/fanotify/fanotify_user.c
> > > +++ b/fs/notify/fanotify/fanotify_user.c
> > > @@ -920,6 +920,20 @@ static int fanotify_test_fid(struct path *path, __kernel_fsid_t *fsid)
> > >         return 0;
> > >  }
> > >
> > > +static int fanotify_events_supported(struct path *path, __u64 mask)
> > > +{
> > > +       /*
> > > +        * Proc is special and various files have special locking rules so
> > > +        * fanotify permission events have high chances of deadlocking the
> > > +        * system. Just disallow them.
> > > +        */
> > > +       if (mask & FANOTIFY_PERM_EVENTS &&
> > > +           !strcmp(path->mnt->mnt_sb->s_type->name, "proc")) {
> > 
> > Better use an SB_I_ flag to forbid permission events on fs?
> 
> So checking s_type->name indeed felt dirty. I don't think we need a
> superblock flag though. I'll probably just go with FS_XXX flag in
> file_system_type.

Would the same apply for some files that backed by sysfs and reside in
/sys?
 
> > 
> > > +               return -EOPNOTSUPP;
> > 
> > I would go with EINVAL following precedent of per filesystem flags
> > check on rename(2), but not insisting.
> 
> I was undecided between EOPNOTSUPP and EINVAL. So let's go with EINVAL.

I was also thinking that EINVAL makes more sense in this particular
case.
 
> > Anyway, following Matthew's man page update for FAN_REPORT_FID,
> > we should also add this as reason for EOPNOTSUPP/EINVAL.
> 
> Good point.

I've followed up Michael in regards to the FAN_REPORT_FID patch series,
but no response as of yet. I'm happy to write the changes for this one
if you like?
Jan Kara May 22, 2019, 9:42 a.m. UTC | #4
On Wed 22-05-19 07:57:18, Matthew Bobrowski wrote:
> On Thu, May 16, 2019 at 10:36:32AM +0200, Jan Kara wrote:
> > On Thu 16-05-19 08:54:37, Amir Goldstein wrote:
> > > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > > ---
> > > >  fs/notify/fanotify/fanotify_user.c | 20 ++++++++++++++++++++
> > > >  1 file changed, 20 insertions(+)
> > > >
> > > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > > > index a90bb19dcfa2..73719949faa6 100644
> > > > --- a/fs/notify/fanotify/fanotify_user.c
> > > > +++ b/fs/notify/fanotify/fanotify_user.c
> > > > @@ -920,6 +920,20 @@ static int fanotify_test_fid(struct path *path, __kernel_fsid_t *fsid)
> > > >         return 0;
> > > >  }
> > > >
> > > > +static int fanotify_events_supported(struct path *path, __u64 mask)
> > > > +{
> > > > +       /*
> > > > +        * Proc is special and various files have special locking rules so
> > > > +        * fanotify permission events have high chances of deadlocking the
> > > > +        * system. Just disallow them.
> > > > +        */
> > > > +       if (mask & FANOTIFY_PERM_EVENTS &&
> > > > +           !strcmp(path->mnt->mnt_sb->s_type->name, "proc")) {
> > > 
> > > Better use an SB_I_ flag to forbid permission events on fs?
> > 
> > So checking s_type->name indeed felt dirty. I don't think we need a
> > superblock flag though. I'll probably just go with FS_XXX flag in
> > file_system_type.
> 
> Would the same apply for some files that backed by sysfs and reside in
> /sys?

So far I'm not aware of similar easy to trigger deadlocks with sysfs. So I
opted for a cautious path and disabled permission events only for proc.
We'll see how that fares.

> > > > +               return -EOPNOTSUPP;
> > > 
> > > I would go with EINVAL following precedent of per filesystem flags
> > > check on rename(2), but not insisting.
> > 
> > I was undecided between EOPNOTSUPP and EINVAL. So let's go with EINVAL.
> 
> I was also thinking that EINVAL makes more sense in this particular
> case.

Good, that's what I used in v2.

> > > Anyway, following Matthew's man page update for FAN_REPORT_FID,
> > > we should also add this as reason for EOPNOTSUPP/EINVAL.
> > 
> > Good point.
> 
> I've followed up Michael in regards to the FAN_REPORT_FID patch series,
> but no response as of yet. I'm happy to write the changes for this one
> if you like?

If you had time for that, that would be nice. Thanks!

								Honza
Matthew Bobrowski May 26, 2019, 11:38 a.m. UTC | #5
On Wed, May 22, 2019 at 11:42:01AM +0200, Jan Kara wrote:
> On Wed 22-05-19 07:57:18, Matthew Bobrowski wrote:
> > On Thu, May 16, 2019 at 10:36:32AM +0200, Jan Kara wrote:
> > > On Thu 16-05-19 08:54:37, Amir Goldstein wrote:
> > > > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > > > ---
> > > > >  fs/notify/fanotify/fanotify_user.c | 20 ++++++++++++++++++++
> > > > >  1 file changed, 20 insertions(+)
> > > > >
> > > > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > > > > index a90bb19dcfa2..73719949faa6 100644
> > > > > --- a/fs/notify/fanotify/fanotify_user.c
> > > > > +++ b/fs/notify/fanotify/fanotify_user.c
> > > > > @@ -920,6 +920,20 @@ static int fanotify_test_fid(struct path *path, __kernel_fsid_t *fsid)
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > +static int fanotify_events_supported(struct path *path, __u64 mask)
> > > > > +{
> > > > > +       /*
> > > > > +        * Proc is special and various files have special locking rules so
> > > > > +        * fanotify permission events have high chances of deadlocking the
> > > > > +        * system. Just disallow them.
> > > > > +        */
> > > > > +       if (mask & FANOTIFY_PERM_EVENTS &&
> > > > > +           !strcmp(path->mnt->mnt_sb->s_type->name, "proc")) {
> > > > 
> > > > Better use an SB_I_ flag to forbid permission events on fs?
> > > 
> > > So checking s_type->name indeed felt dirty. I don't think we need a
> > > superblock flag though. I'll probably just go with FS_XXX flag in
> > > file_system_type.
> > 
> > Would the same apply for some files that backed by sysfs and reside in
> > /sys?
> 
> So far I'm not aware of similar easy to trigger deadlocks with sysfs. So I
> opted for a cautious path and disabled permission events only for proc.
> We'll see how that fares.
> 
> > > > > +               return -EOPNOTSUPP;
> > > > 
> > > > I would go with EINVAL following precedent of per filesystem flags
> > > > check on rename(2), but not insisting.
> > > 
> > > I was undecided between EOPNOTSUPP and EINVAL. So let's go with EINVAL.
> > 
> > I was also thinking that EINVAL makes more sense in this particular
> > case.
> 
> Good, that's what I used in v2.
> 
> > > > Anyway, following Matthew's man page update for FAN_REPORT_FID,
> > > > we should also add this as reason for EOPNOTSUPP/EINVAL.
> > > 
> > > Good point.
> > 
> > I've followed up Michael in regards to the FAN_REPORT_FID patch series,
> > but no response as of yet. I'm happy to write the changes for this one
> > if you like?
> 
> If you had time for that, that would be nice. Thanks!

Simple. I will write the changes and send it through for review.

Patch
diff mbox series

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index a90bb19dcfa2..73719949faa6 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -920,6 +920,20 @@  static int fanotify_test_fid(struct path *path, __kernel_fsid_t *fsid)
 	return 0;
 }
 
+static int fanotify_events_supported(struct path *path, __u64 mask)
+{
+	/*
+	 * Proc is special and various files have special locking rules so
+	 * fanotify permission events have high chances of deadlocking the
+	 * system. Just disallow them.
+	 */
+	if (mask & FANOTIFY_PERM_EVENTS &&
+	    !strcmp(path->mnt->mnt_sb->s_type->name, "proc")) {
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
 static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 			    int dfd, const char  __user *pathname)
 {
@@ -1018,6 +1032,12 @@  static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	if (ret)
 		goto fput_and_out;
 
+	if (flags & FAN_MARK_ADD) {
+		ret = fanotify_events_supported(&path, mask);
+		if (ret)
+			goto path_put_and_out;
+	}
+
 	if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
 		ret = fanotify_test_fid(&path, &__fsid);
 		if (ret)