From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 115ACC433EF for ; Sun, 28 Nov 2021 16:56:01 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1358588AbhK1Q7Q (ORCPT ); Sun, 28 Nov 2021 11:59:16 -0500 Received: from alexa-out-sd-02.qualcomm.com ([199.106.114.39]:34046 "EHLO alexa-out-sd-02.qualcomm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234648AbhK1Q5M (ORCPT ); Sun, 28 Nov 2021 11:57:12 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; i=@quicinc.com; q=dns/txt; s=qcdkim; t=1638118436; x=1669654436; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=BMBl9O3+J29K/k8p9pOXG0M93tcMkeeb1t+hISUHhEE=; b=Z84zVn5Diga11q6hAtmtLRvJcfYUHdPPNEtXAERHtVUmtRzz3X+1npX9 tx/mUdOA2DyFCFiMbn0LsoWDOF7tZtZvjKGDxiF+QhaqA2eMcILYhHf1I 2SYVjIsEoqnG1ihSCjj+G0yGlZBUQyjSNUbe1vZ/VAG8K4YEvWxg2p8q+ k=; Received: from unknown (HELO ironmsg03-sd.qualcomm.com) ([10.53.140.143]) by alexa-out-sd-02.qualcomm.com with ESMTP; 28 Nov 2021 08:53:56 -0800 X-QCInternal: smtphost Received: from nasanex01c.na.qualcomm.com ([10.47.97.222]) by ironmsg03-sd.qualcomm.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Nov 2021 08:53:55 -0800 Received: from nalasex01a.na.qualcomm.com (10.47.209.196) by nasanex01c.na.qualcomm.com (10.47.97.222) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.922.19; Sun, 28 Nov 2021 08:53:55 -0800 Received: from jackp-linux.qualcomm.com (10.80.80.8) by nalasex01a.na.qualcomm.com (10.47.209.196) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.922.19; Sun, 28 Nov 2021 08:53:55 -0800 Date: Sun, 28 Nov 2021 08:53:50 -0800 From: Jack Pham To: Udipto Goswami CC: Felipe Balbi , Greg Kroah-Hartman , John Keeping , , Pratham Pratap , Pavankumar Kondeti Subject: Re: [PATCH v4] usb: f_fs: Fix use-after-free for epfile Message-ID: <20211128165350.GA23891@jackp-linux.qualcomm.com> References: <1637936072-32263-1-git-send-email-quic_ugoswami@quicinc.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <1637936072-32263-1-git-send-email-quic_ugoswami@quicinc.com> User-Agent: Mutt/1.9.4 (2018-02-28) X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01b.na.qualcomm.com (10.46.141.250) To nalasex01a.na.qualcomm.com (10.47.209.196) Precedence: bulk List-ID: X-Mailing-List: linux-usb@vger.kernel.org Hi Udipto, On Fri, Nov 26, 2021 at 07:44:32PM +0530, Udipto Goswami wrote: > 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. > > Signed-off-by: Pratham Pratap > Co-developed-by: Udipto Goswami > Signed-off-by: Udipto Goswami > --- > v4: Fix undeclared epfile error in ffs_func_eps_disable. > > drivers/usb/gadget/function/f_fs.c | 49 +++++++++++++++++++++++++++----------- > 1 file changed, 35 insertions(+), 14 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c > index 3c584da..2a634d7 100644 > --- a/drivers/usb/gadget/function/f_fs.c > +++ b/drivers/usb/gadget/function/f_fs.c > @@ -1711,16 +1711,23 @@ 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 { > @@ -1767,14 +1774,25 @@ 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); > > if (ffs->ffs_eventfd) > eventfd_ctx_put(ffs->ffs_eventfd); > @@ -1919,22 +1937,25 @@ 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 *epfiles; This particular instance could have been kept named "epfile" (singular) as it functions more like an iterator in this function. It'll also help avoid needing to change the chunk below within the while loop. > + unsigned short count; > unsigned long flags; > > spin_lock_irqsave(&func->ffs->eps_lock, flags); > + count = func->ffs->eps_count; > + epfiles = func->ffs->epfiles; > + ep = func->eps; > while (count--) { > /* pending requests get nuked */ > if (ep->ep) > usb_ep_disable(ep->ep); > ++ep; > > - if (epfile) { > - epfile->ep = NULL; > - __ffs_epfile_read_buffer_free(epfile); > - ++epfile; > + if (epfiles) { > + epfiles->ep = NULL; > + __ffs_epfile_read_buffer_free(epfiles); > + ++epfiles; > } > } > spin_unlock_irqrestore(&func->ffs->eps_lock, flags); Thanks, Jack