lkml.org 
[lkml]   [2016]   [Nov]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    Subject[PATCH 3.16 310/346] fanotify: fix list corruption in fanotify_get_response()
    3.16.39-rc1 review patch.  If anyone has any objections, please let me know.

    ------------------

    From: Jan Kara <jack@suse.cz>

    commit 96d41019e3ac55f6f0115b0ce97e4f24a3d636d2 upstream.

    fanotify_get_response() calls fsnotify_remove_event() when it finds that
    group is being released from fanotify_release() (bypass_perm is set).

    However the event it removes need not be only in the group's notification
    queue but it can have already moved to access_list (userspace read the
    event before closing the fanotify instance fd) which is protected by a
    different lock. Thus when fsnotify_remove_event() races with
    fanotify_release() operating on access_list, the list can get corrupted.

    Fix the problem by moving all the logic removing permission events from
    the lists to one place - fanotify_release().

    Fixes: 5838d4442bd5 ("fanotify: fix double free of pending permission events")
    Link: http://lkml.kernel.org/r/1473797711-14111-3-git-send-email-jack@suse.cz
    Signed-off-by: Jan Kara <jack@suse.cz>
    Reported-by: Miklos Szeredi <mszeredi@redhat.com>
    Tested-by: Miklos Szeredi <mszeredi@redhat.com>
    Reviewed-by: Miklos Szeredi <mszeredi@redhat.com>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
    [bwh: Backported to 3.16:
    - s/fsnotify_remove_first_event/fsnotify_remove_notify_event/
    - Adjust context]
    Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
    ---
    fs/notify/fanotify/fanotify.c | 13 +------------
    fs/notify/fanotify/fanotify_user.c | 36 ++++++++++++++++++++++++------------
    fs/notify/notification.c | 15 ---------------
    include/linux/fsnotify_backend.h | 3 ---
    4 files changed, 25 insertions(+), 42 deletions(-)

    --- a/fs/notify/fanotify/fanotify.c
    +++ b/fs/notify/fanotify/fanotify.c
    @@ -67,18 +67,7 @@ static int fanotify_get_response(struct

    pr_debug("%s: group=%p event=%p\n", __func__, group, event);

    - wait_event(group->fanotify_data.access_waitq, event->response ||
    - atomic_read(&group->fanotify_data.bypass_perm));
    -
    - if (!event->response) { /* bypass_perm set */
    - /*
    - * Event was canceled because group is being destroyed. Remove
    - * it from group's event list because we are responsible for
    - * freeing the permission event.
    - */
    - fsnotify_remove_event(group, &event->fae.fse);
    - return 0;
    - }
    + wait_event(group->fanotify_data.access_waitq, event->response);

    /* userspace responded, convert to something usable */
    switch (event->response) {
    --- a/fs/notify/fanotify/fanotify_user.c
    +++ b/fs/notify/fanotify/fanotify_user.c
    @@ -358,16 +358,20 @@ static int fanotify_release(struct inode

    #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
    struct fanotify_perm_event_info *event, *next;
    + struct fsnotify_event *fsn_event;

    /*
    - * There may be still new events arriving in the notification queue
    - * but since userspace cannot use fanotify fd anymore, no event can
    - * enter or leave access_list by now.
    + * Stop new events from arriving in the notification queue. since
    + * userspace cannot use fanotify fd anymore, no event can enter or
    + * leave access_list by now either.
    */
    - spin_lock(&group->fanotify_data.access_lock);
    -
    - atomic_inc(&group->fanotify_data.bypass_perm);
    + fsnotify_group_stop_queueing(group);

    + /*
    + * Process all permission events on access_list and notification queue
    + * and simulate reply from userspace.
    + */
    + spin_lock(&group->fanotify_data.access_lock);
    list_for_each_entry_safe(event, next, &group->fanotify_data.access_list,
    fae.fse.list) {
    pr_debug("%s: found group=%p event=%p\n", __func__, group,
    @@ -379,12 +383,21 @@ static int fanotify_release(struct inode
    spin_unlock(&group->fanotify_data.access_lock);

    /*
    - * Since bypass_perm is set, newly queued events will not wait for
    - * access response. Wake up the already sleeping ones now.
    - * synchronize_srcu() in fsnotify_destroy_group() will wait for all
    - * processes sleeping in fanotify_handle_event() waiting for access
    - * response and thus also for all permission events to be freed.
    + * Destroy all non-permission events. For permission events just
    + * dequeue them and set the response. They will be freed once the
    + * response is consumed and fanotify_get_response() returns.
    */
    + mutex_lock(&group->notification_mutex);
    + while (!fsnotify_notify_queue_is_empty(group)) {
    + fsn_event = fsnotify_remove_notify_event(group);
    + if (!(fsn_event->mask & FAN_ALL_PERM_EVENTS))
    + fsnotify_destroy_event(group, fsn_event);
    + else
    + FANOTIFY_PE(fsn_event)->response = FAN_ALLOW;
    + }
    + mutex_unlock(&group->notification_mutex);
    +
    + /* Response for all permission events it set, wakeup waiters */
    wake_up(&group->fanotify_data.access_waitq);
    #endif

    @@ -742,7 +755,6 @@ SYSCALL_DEFINE2(fanotify_init, unsigned
    spin_lock_init(&group->fanotify_data.access_lock);
    init_waitqueue_head(&group->fanotify_data.access_waitq);
    INIT_LIST_HEAD(&group->fanotify_data.access_list);
    - atomic_set(&group->fanotify_data.bypass_perm, 0);
    #endif
    switch (flags & FAN_ALL_CLASS_BITS) {
    case FAN_CLASS_NOTIF:
    --- a/fs/notify/notification.c
    +++ b/fs/notify/notification.c
    @@ -132,21 +132,6 @@ queue:
    }

    /*
    - * Remove @event from group's notification queue. It is the responsibility of
    - * the caller to destroy the event.
    - */
    -void fsnotify_remove_event(struct fsnotify_group *group,
    - struct fsnotify_event *event)
    -{
    - mutex_lock(&group->notification_mutex);
    - if (!list_empty(&event->list)) {
    - list_del_init(&event->list);
    - group->q_len--;
    - }
    - mutex_unlock(&group->notification_mutex);
    -}
    -
    -/*
    * Remove and return the first event from the notification list. It is the
    * responsibility of the caller to destroy the obtained event
    */
    --- a/include/linux/fsnotify_backend.h
    +++ b/include/linux/fsnotify_backend.h
    @@ -182,7 +182,6 @@ struct fsnotify_group {
    spinlock_t access_lock;
    struct list_head access_list;
    wait_queue_head_t access_waitq;
    - atomic_t bypass_perm;
    #endif /* CONFIG_FANOTIFY_ACCESS_PERMISSIONS */
    int f_flags;
    unsigned int max_marks;
    @@ -329,8 +328,6 @@ extern int fsnotify_add_notify_event(str
    struct fsnotify_event *event,
    int (*merge)(struct list_head *,
    struct fsnotify_event *));
    -/* Remove passed event from groups notification queue */
    -extern void fsnotify_remove_event(struct fsnotify_group *group, struct fsnotify_event *event);
    /* true if the group notification queue is empty */
    extern bool fsnotify_notify_queue_is_empty(struct fsnotify_group *group);
    /* return, but do not dequeue the first event on the notification queue */
    \
     
     \ /
      Last update: 2016-11-14 04:25    [W:4.397 / U:0.472 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site