* [PATCH] fanotify: Disallow permission events for proc filesystem
@ 2019-05-15 19:33 Jan Kara
2019-05-16 5:54 ` Amir Goldstein
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kara @ 2019-05-15 19:33 UTC (permalink / raw)
To: linux-fsdevel; +Cc: Amir Goldstein, Jan Kara
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(+)
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)
--
2.16.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] fanotify: Disallow permission events for proc filesystem
2019-05-15 19:33 [PATCH] fanotify: Disallow permission events for proc filesystem Jan Kara
@ 2019-05-16 5:54 ` Amir Goldstein
2019-05-16 8:36 ` Jan Kara
0 siblings, 1 reply; 6+ messages in thread
From: Amir Goldstein @ 2019-05-16 5:54 UTC (permalink / raw)
To: Jan Kara; +Cc: linux-fsdevel, Matthew Bobrowski
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fanotify: Disallow permission events for proc filesystem
2019-05-16 5:54 ` Amir Goldstein
@ 2019-05-16 8:36 ` Jan Kara
2019-05-21 21:57 ` Matthew Bobrowski
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kara @ 2019-05-16 8:36 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Jan Kara, linux-fsdevel, Matthew Bobrowski
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
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fanotify: Disallow permission events for proc filesystem
2019-05-16 8:36 ` Jan Kara
@ 2019-05-21 21:57 ` Matthew Bobrowski
2019-05-22 9:42 ` Jan Kara
0 siblings, 1 reply; 6+ messages in thread
From: Matthew Bobrowski @ 2019-05-21 21:57 UTC (permalink / raw)
To: Jan Kara; +Cc: Amir Goldstein, linux-fsdevel
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?
--
Matthew Bobrowski
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fanotify: Disallow permission events for proc filesystem
2019-05-21 21:57 ` Matthew Bobrowski
@ 2019-05-22 9:42 ` Jan Kara
2019-05-26 11:38 ` Matthew Bobrowski
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kara @ 2019-05-22 9:42 UTC (permalink / raw)
To: Matthew Bobrowski; +Cc: Jan Kara, Amir Goldstein, linux-fsdevel
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
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fanotify: Disallow permission events for proc filesystem
2019-05-22 9:42 ` Jan Kara
@ 2019-05-26 11:38 ` Matthew Bobrowski
0 siblings, 0 replies; 6+ messages in thread
From: Matthew Bobrowski @ 2019-05-26 11:38 UTC (permalink / raw)
To: Jan Kara; +Cc: Amir Goldstein, linux-fsdevel
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.
--
Matthew Bobrowski
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-05-26 11:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-15 19:33 [PATCH] fanotify: Disallow permission events for proc filesystem Jan Kara
2019-05-16 5:54 ` Amir Goldstein
2019-05-16 8:36 ` Jan Kara
2019-05-21 21:57 ` Matthew Bobrowski
2019-05-22 9:42 ` Jan Kara
2019-05-26 11:38 ` Matthew Bobrowski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).