On Wed, Mar 20, 2019 at 4:30 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Wed 20-03-19 15:46:20, Amir Goldstein wrote:
> > On Wed, Mar 20, 2019 at 3:16 PM Jan Kara <jack@xxxxxxx> wrote:
> > > recently, one of our customers has reported a deadlock with fanotify. The
> > > analysis has shown that a process has put (likely by mistake) FAN_OPEN_PERM
> > > mark on /proc and / filesystem. That resulted in a deadlock like follows:
> > >
> > > process 1: process 2: process 3:
> > > open("/proc/process 2/maps")
> > > - blocks waiting for response to
> > > FAN_OPEN_PERM event
> > >
> > > exec(2)
> > > __do_execve_file()
> > > - grabs current->signal->cred_guard_mutex
> > > do_open_execat()
> > > - blocks waiting for response to
> > > FAN_OPEN_PERM event
> > >
> > > reads fanotify event
> > > generated by process 1
> > > create_fd()
> > > dentry_open()
> > > proc_maps_open()
> > > blocks on
> > > cred_guard_mutex of process 2.
> > >
> > > Now this is not the only case where you can setup fanotify permissions
> > > events so that your listener deadlocks but I'd argue that this case is
> > > especially nasty and it is unrealistic to expect from userspace that it
> > > would be able to implement fanotify listener in such a way that is
> > > deadlock-free for proc filesystem since the lock dependencies there are
> > > very different. So how about we just forbid placing marks with fanotify
> > > permission events on proc? Any other virtual filesystem we should exclude?
> > >
> >
> > I bet if we forbid placing marks on /proc, some apps would break.
>
> Well, I didn't mean all marks, just the permission ones. I'm not sure there
> are apps that place permission events on /proc...
>
Maybe not intentionally.
I once tested a few fanotify based AntiVirus solutions.
In some of them, setting an "Exclude path" on some mount point
would cause mark to not be set on that path, but for one in particular,
the mark was still being set on the mount so path pattern filtering was
done after receiving the events.
I did not check whether /proc was blacklisted out of the box or if it
could be marked/excluded from scan.
IMO, assuming that all AntiVirus vendors blacklist all virtual filesystems
is an assumption that we need to validate.
[CC Marko from F-Secure for commenting on the above.]
> > I always though that allowing O_PATH in event_f_flags can make
> > sense for some apps.
> >
> > What if instead of forbiding marks of /proc, we would force those
> > marks to use O_PATH for fd creation. Some of the functionality
> > will remain. Apps are less likely to break. Deadlocks will be less
> > likely, although maybe still possible.
>
> Yes, that's another option. But if this is automatic, it is going to be
> confusing to potential users - report O_PATH fd if getting normal fd is
> dangerous isn't great. And also the deadlocks are there only for permission
> events so there's no strong reason to restrict normal ones.
>
Of course, we can do this hack only for reading permission events
for virtual filesystems. The question is what would be more surprising
to existing apps:
- marks for permission events no longer working on /proc
- fd from permission events no longer readable from /proc
- something completely different
Thanks,
Amir.
