lkml.org 
[lkml]   [2022]   [Feb]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 4.19 25/49] usb: f_fs: Fix use-after-free for epfile
    Date
    From: Udipto Goswami <quic_ugoswami@quicinc.com>

    [ Upstream commit ebe2b1add1055b903e2acd86b290a85297edc0b3 ]

    Consider a case where ffs_func_eps_disable is called from
    ffs_func_disable as part of composition switch and at the
    same time ffs_epfile_release get called from userspace.
    ffs_epfile_release will free up the read buffer and call
    ffs_data_closed which in turn destroys ffs->epfiles and
    mark it as NULL. While this was happening the driver has
    already initialized the local epfile in ffs_func_eps_disable
    which is now freed and waiting to acquire the spinlock. Once
    spinlock is acquired the driver proceeds with the stale value
    of epfile and tries to free the already freed read buffer
    causing use-after-free.

    Following is the illustration of the race:

    CPU1 CPU2

    ffs_func_eps_disable
    epfiles (local copy)
    ffs_epfile_release
    ffs_data_closed
    if (last file closed)
    ffs_data_reset
    ffs_data_clear
    ffs_epfiles_destroy
    spin_lock
    dereference epfiles

    Fix this races by taking epfiles local copy & assigning it under
    spinlock and if epfiles(local) is null then update it in ffs->epfiles
    then finally destroy it.
    Extending the scope further from the race, protecting the ep related
    structures, and concurrent accesses.

    Fixes: a9e6f83c2df1 ("usb: gadget: f_fs: stop sleeping in ffs_func_eps_disable")
    Co-developed-by: Udipto Goswami <quic_ugoswami@quicinc.com>
    Reviewed-by: John Keeping <john@metanate.com>
    Signed-off-by: Pratham Pratap <quic_ppratap@quicinc.com>
    Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com>
    Link: https://lore.kernel.org/r/1643256595-10797-1-git-send-email-quic_ugoswami@quicinc.com
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    Signed-off-by: Sasha Levin <sashal@kernel.org>
    ---
    drivers/usb/gadget/function/f_fs.c | 56 ++++++++++++++++++++++--------
    1 file changed, 42 insertions(+), 14 deletions(-)

    diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
    index 9271a7009a00f..49eb4e3c760f4 100644
    --- a/drivers/usb/gadget/function/f_fs.c
    +++ b/drivers/usb/gadget/function/f_fs.c
    @@ -1636,16 +1636,24 @@ static void ffs_data_put(struct ffs_data *ffs)

    static void ffs_data_closed(struct ffs_data *ffs)
    {
    + struct ffs_epfile *epfiles;
    + unsigned long flags;
    +
    ENTER();

    if (atomic_dec_and_test(&ffs->opened)) {
    if (ffs->no_disconnect) {
    ffs->state = FFS_DEACTIVATED;
    - if (ffs->epfiles) {
    - ffs_epfiles_destroy(ffs->epfiles,
    - ffs->eps_count);
    - ffs->epfiles = NULL;
    - }
    + spin_lock_irqsave(&ffs->eps_lock, flags);
    + epfiles = ffs->epfiles;
    + ffs->epfiles = NULL;
    + spin_unlock_irqrestore(&ffs->eps_lock,
    + flags);
    +
    + if (epfiles)
    + ffs_epfiles_destroy(epfiles,
    + ffs->eps_count);
    +
    if (ffs->setup_state == FFS_SETUP_PENDING)
    __ffs_ep0_stall(ffs);
    } else {
    @@ -1692,14 +1700,27 @@ static struct ffs_data *ffs_data_new(const char *dev_name)

    static void ffs_data_clear(struct ffs_data *ffs)
    {
    + struct ffs_epfile *epfiles;
    + unsigned long flags;
    +
    ENTER();

    ffs_closed(ffs);

    BUG_ON(ffs->gadget);

    - if (ffs->epfiles) {
    - ffs_epfiles_destroy(ffs->epfiles, ffs->eps_count);
    + spin_lock_irqsave(&ffs->eps_lock, flags);
    + epfiles = ffs->epfiles;
    + ffs->epfiles = NULL;
    + spin_unlock_irqrestore(&ffs->eps_lock, flags);
    +
    + /*
    + * potential race possible between ffs_func_eps_disable
    + * & ffs_epfile_release therefore maintaining a local
    + * copy of epfile will save us from use-after-free.
    + */
    + if (epfiles) {
    + ffs_epfiles_destroy(epfiles, ffs->eps_count);
    ffs->epfiles = NULL;
    }

    @@ -1847,12 +1868,15 @@ static void ffs_epfiles_destroy(struct ffs_epfile *epfiles, unsigned count)

    static void ffs_func_eps_disable(struct ffs_function *func)
    {
    - struct ffs_ep *ep = func->eps;
    - struct ffs_epfile *epfile = func->ffs->epfiles;
    - unsigned count = func->ffs->eps_count;
    + struct ffs_ep *ep;
    + struct ffs_epfile *epfile;
    + unsigned short count;
    unsigned long flags;

    spin_lock_irqsave(&func->ffs->eps_lock, flags);
    + count = func->ffs->eps_count;
    + epfile = func->ffs->epfiles;
    + ep = func->eps;
    while (count--) {
    /* pending requests get nuked */
    if (likely(ep->ep))
    @@ -1870,14 +1894,18 @@ static void ffs_func_eps_disable(struct ffs_function *func)

    static int ffs_func_eps_enable(struct ffs_function *func)
    {
    - struct ffs_data *ffs = func->ffs;
    - struct ffs_ep *ep = func->eps;
    - struct ffs_epfile *epfile = ffs->epfiles;
    - unsigned count = ffs->eps_count;
    + struct ffs_data *ffs;
    + struct ffs_ep *ep;
    + struct ffs_epfile *epfile;
    + unsigned short count;
    unsigned long flags;
    int ret = 0;

    spin_lock_irqsave(&func->ffs->eps_lock, flags);
    + ffs = func->ffs;
    + ep = func->eps;
    + epfile = ffs->epfiles;
    + count = ffs->eps_count;
    while(count--) {
    ep->ep->driver_data = ep;

    --
    2.34.1


    \
     
     \ /
      Last update: 2022-02-14 10:40    [W:4.057 / U:2.560 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site