lkml.org 
[lkml]   [2016]   [Nov]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 4.8 017/138] gpio: GPIO_GET_LINE{HANDLE,EVENT}_IOCTL: Fix file descriptor leak
    Date
    4.8-stable review patch.  If anyone has any objections, please let me know.

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

    From: Lars-Peter Clausen <lars@metafoo.de>

    commit 953b956a2e6d35298e684f251bad98ea6c96f982 upstream.

    When allocating a new line handle or event a file is allocated that it is
    associated to. The file is attached to a file descriptor of the current
    process and the file descriptor is returned to userspace using
    copy_to_user(). If this copy operation fails the line handle or event
    allocation is aborted, all acquired resources are freed and an error is
    returned.

    But the file struct is not freed and left attached to the userspace
    application and even though the file descriptor number was not copied it is
    trivial to guess. If a userspace application performs a IOCTL on such a
    left over file descriptor it will trigger a use-after-free and if the file
    descriptor is closed (latest when the application exits) a double-free is
    triggered.

    anon_inode_getfd() performs 3 tasks, allocate a file struct, allocate a
    file descriptor for the current process and install the file struct in the
    file descriptor. As soon as the file struct is installed in the file
    descriptor it is accessible by userspace (even if the IOCTL itself hasn't
    completed yet), this means uninstalling the fd on the error path is not an
    option, since userspace might already got a reference to the file.

    Instead anon_inode_getfd() needs to be broken into its individual steps.
    The allocation of the file struct and file descriptor is done first, then
    the copy_to_user() is executed and only if it succeeds the file is
    installed.

    Since the file struct is reference counted it can not be just freed, but
    its reference needs to be dropped, which will also call the release()
    callback, which will free the state attached to the file. So in this case
    the normal error cleanup path should not be taken.

    Fixes: d932cd49182f ("gpio: free handles in fringe cases")
    Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
    Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

    ---
    drivers/gpio/gpiolib.c | 57 ++++++++++++++++++++++++++++++++++++++-----------
    1 file changed, 45 insertions(+), 12 deletions(-)

    --- a/drivers/gpio/gpiolib.c
    +++ b/drivers/gpio/gpiolib.c
    @@ -21,6 +21,7 @@
    #include <linux/uaccess.h>
    #include <linux/compat.h>
    #include <linux/anon_inodes.h>
    +#include <linux/file.h>
    #include <linux/kfifo.h>
    #include <linux/poll.h>
    #include <linux/timekeeping.h>
    @@ -421,6 +422,7 @@ static int linehandle_create(struct gpio
    {
    struct gpiohandle_request handlereq;
    struct linehandle_state *lh;
    + struct file *file;
    int fd, i, ret;

    if (copy_from_user(&handlereq, ip, sizeof(handlereq)))
    @@ -497,26 +499,41 @@ static int linehandle_create(struct gpio
    i--;
    lh->numdescs = handlereq.lines;

    - fd = anon_inode_getfd("gpio-linehandle",
    - &linehandle_fileops,
    - lh,
    - O_RDONLY | O_CLOEXEC);
    + fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
    if (fd < 0) {
    ret = fd;
    goto out_free_descs;
    }

    + file = anon_inode_getfile("gpio-linehandle",
    + &linehandle_fileops,
    + lh,
    + O_RDONLY | O_CLOEXEC);
    + if (IS_ERR(file)) {
    + ret = PTR_ERR(file);
    + goto out_put_unused_fd;
    + }
    +
    handlereq.fd = fd;
    if (copy_to_user(ip, &handlereq, sizeof(handlereq))) {
    - ret = -EFAULT;
    - goto out_free_descs;
    + /*
    + * fput() will trigger the release() callback, so do not go onto
    + * the regular error cleanup path here.
    + */
    + fput(file);
    + put_unused_fd(fd);
    + return -EFAULT;
    }

    + fd_install(fd, file);
    +
    dev_dbg(&gdev->dev, "registered chardev handle for %d lines\n",
    lh->numdescs);

    return 0;

    +out_put_unused_fd:
    + put_unused_fd(fd);
    out_free_descs:
    for (; i >= 0; i--)
    gpiod_free(lh->descs[i]);
    @@ -719,6 +736,7 @@ static int lineevent_create(struct gpio_
    struct gpioevent_request eventreq;
    struct lineevent_state *le;
    struct gpio_desc *desc;
    + struct file *file;
    u32 offset;
    u32 lflags;
    u32 eflags;
    @@ -813,23 +831,38 @@ static int lineevent_create(struct gpio_
    if (ret)
    goto out_free_desc;

    - fd = anon_inode_getfd("gpio-event",
    - &lineevent_fileops,
    - le,
    - O_RDONLY | O_CLOEXEC);
    + fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
    if (fd < 0) {
    ret = fd;
    goto out_free_irq;
    }

    + file = anon_inode_getfile("gpio-event",
    + &lineevent_fileops,
    + le,
    + O_RDONLY | O_CLOEXEC);
    + if (IS_ERR(file)) {
    + ret = PTR_ERR(file);
    + goto out_put_unused_fd;
    + }
    +
    eventreq.fd = fd;
    if (copy_to_user(ip, &eventreq, sizeof(eventreq))) {
    - ret = -EFAULT;
    - goto out_free_irq;
    + /*
    + * fput() will trigger the release() callback, so do not go onto
    + * the regular error cleanup path here.
    + */
    + fput(file);
    + put_unused_fd(fd);
    + return -EFAULT;
    }

    + fd_install(fd, file);
    +
    return 0;

    +out_put_unused_fd:
    + put_unused_fd(fd);
    out_free_irq:
    free_irq(le->irq, le);
    out_free_desc:

    \
     
     \ /
      Last update: 2016-11-09 12:36    [W:4.424 / U:0.356 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site