linux-kernel - Re: [PATCH] fanotify: use killable wait for waiting response for permission events

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20180821144315.GC11261@quack2.suse.cz>
Date:   Tue, 21 Aug 2018 16:43:15 +0200
From:   Jan Kara <jack@...e.cz>
To:     Konstantin Khlebnikov <khlebnikov@...dex-team.ru>
Cc:     Jan Kara <jack@...e.cz>, linux-fsdevel@...r.kernel.org,
        Amir Goldstein <amir73il@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fanotify: use killable wait for waiting response for
 permission events

On Tue 21-08-18 16:42:26, Konstantin Khlebnikov wrote:
> On 20.08.2018 13:53, Jan Kara wrote:
> > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > > index eb4e75175cfb..7a0c37790c89 100644
> > > --- a/fs/notify/fanotify/fanotify.c
> > > +++ b/fs/notify/fanotify/fanotify.c
> > > @@ -64,7 +64,27 @@ static int fanotify_get_response(struct fsnotify_group *group,
> > >   	pr_debug("%s: group=%p event=%p\n", __func__, group, event);
> > > -	wait_event(group->fanotify_data.access_waitq, event->response);
> > > +	ret = wait_event_killable(group->fanotify_data.access_waitq,
> > > +				  event->response);
> > > +	if (ret) {
> > > +		/* Try to remove pending event from the queue */
> > > +		spin_lock(&group->notification_lock);
> > > +		if (!list_empty(&event->fae.fse.list))
> > > +			list_del_init(&event->fae.fse.list);
> > 
> > Here you forget to decrement group->q_len like
> > fsnotify_remove_first_event() does.
> > 
> 
> Yep

Actually only if this was the list of events to report to userspace. If the
event was on a list of events already reported but not responded to,
group->q_len should not be touched.

> > > +		else
> > > +			ret = 0;
> > > +		spin_unlock(&group->notification_lock);
> > 
> > So the above check for list_empty can hit either when response is just
> > being processed (and then we'll be woken up very soon) or when the event is
> > just in the process of being copied from event queue to userspace (in which
> > case we are in the same situation as in the old code). So it would be
> > weird that in rare cases wait would not be really killable. I think we
> > could detect this situation in fanotify_read() before adding event to
> > access_list and just wakeup waiter in fanotify_get_response() again and
> > avoid reporting the event to userspace. Hmm?
> 
> I've missed that move from list to list in fanotify_read().
> 
> So, fanotify_read needs event alive for a long time - copy_to_user might
> block forever.

It might block for a long time due to page fault. That is correct.

> We have to transfer ownership and destroy event in fanotify_read.
> I'll try this approach.

I'm open to that if you come up with something reasonably simple. But you
need to somehow communicate back the response and that used to be a mess
and that's why we ended up with permission events being completely handled
by the process generating them...

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR