| Message ID | 1637905644-32618-1-git-send-email-quic_ugoswami@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] usb: f_fs: Fix use-after-free for epfile | expand |
Hi Udipto,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on usb/usb-testing]
[also build test ERROR on balbi-usb/testing/next peter-chen-usb/for-usb-next v5.16-rc2 next-20211126]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Udipto-Goswami/usb-f_fs-Fix-use-after-free-for-epfile/20211126-135108
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20211126/202111261849.Pw2UJeFw-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/5bcd8f0f07e0a91b72a538d1d76369ed4d410901
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Udipto-Goswami/usb-f_fs-Fix-use-after-free-for-epfile/20211126-135108
git checkout 5bcd8f0f07e0a91b72a538d1d76369ed4d410901
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/usb/gadget/function/f_fs.c: In function 'ffs_func_eps_disable':
>> drivers/usb/gadget/function/f_fs.c:1958:27: error: 'epfile' undeclared (first use in this function); did you mean 'epfiles'?
1958 | ++epfile;
| ^~~~~~
| epfiles
drivers/usb/gadget/function/f_fs.c:1958:27: note: each undeclared identifier is reported only once for each function it appears in
vim +1958 drivers/usb/gadget/function/f_fs.c
ddf8abd2599491c drivers/usb/gadget/f_fs.c Michal Nazarewicz 2010-05-05 1937
ddf8abd2599491c drivers/usb/gadget/f_fs.c Michal Nazarewicz 2010-05-05 1938 static void ffs_func_eps_disable(struct ffs_function *func)
ddf8abd2599491c drivers/usb/gadget/f_fs.c Michal Nazarewicz 2010-05-05 1939 {
5bcd8f0f07e0a91 drivers/usb/gadget/function/f_fs.c Udipto Goswami 2021-11-26 1940 struct ffs_ep *ep;
5bcd8f0f07e0a91 drivers/usb/gadget/function/f_fs.c Udipto Goswami 2021-11-26 1941 struct ffs_epfile *epfiles;
5bcd8f0f07e0a91 drivers/usb/gadget/function/f_fs.c Udipto Goswami 2021-11-26 1942 unsigned short count;
ddf8abd2599491c drivers/usb/gadget/f_fs.c Michal Nazarewicz 2010-05-05 1943 unsigned long flags;
ddf8abd2599491c drivers/usb/gadget/f_fs.c Michal Nazarewicz 2010-05-05 1944
9353afbbfa7b107 drivers/usb/gadget/function/f_fs.c Michal Nazarewicz 2016-05-21 1945 spin_lock_irqsave(&func->ffs->eps_lock, flags);
5bcd8f0f07e0a91 drivers/usb/gadget/function/f_fs.c Udipto Goswami 2021-11-26 1946 count = func->ffs->eps_count;
5bcd8f0f07e0a91 drivers/usb/gadget/function/f_fs.c Udipto Goswami 2021-11-26 1947 epfiles = func->ffs->epfiles;
5bcd8f0f07e0a91 drivers/usb/gadget/function/f_fs.c Udipto Goswami 2021-11-26 1948 ep = func->eps;
08f37148b6a915a drivers/usb/gadget/function/f_fs.c Vincent Pelletier 2017-01-09 1949 while (count--) {
ddf8abd2599491c drivers/usb/gadget/f_fs.c Michal Nazarewicz 2010-05-05 1950 /* pending requests get nuked */
8704fd73bf5658b drivers/usb/gadget/function/f_fs.c Greg Kroah-Hartman 2020-11-27 1951 if (ep->ep)
ddf8abd2599491c drivers/usb/gadget/f_fs.c Michal Nazarewicz 2010-05-05 1952 usb_ep_disable(ep->ep);
ddf8abd2599491c drivers/usb/gadget/f_fs.c Michal Nazarewicz 2010-05-05 1953 ++ep;
18d6b32fca3841f drivers/usb/gadget/function/f_fs.c Robert Baldyga 2014-12-18 1954
5bcd8f0f07e0a91 drivers/usb/gadget/function/f_fs.c Udipto Goswami 2021-11-26 1955 if (epfiles) {
5bcd8f0f07e0a91 drivers/usb/gadget/function/f_fs.c Udipto Goswami 2021-11-26 1956 epfiles->ep = NULL;
5bcd8f0f07e0a91 drivers/usb/gadget/function/f_fs.c Udipto Goswami 2021-11-26 1957 __ffs_epfile_read_buffer_free(epfiles);
ddf8abd2599491c drivers/usb/gadget/f_fs.c Michal Nazarewicz 2010-05-05 @1958 ++epfile;
18d6b32fca3841f drivers/usb/gadget/function/f_fs.c Robert Baldyga 2014-12-18 1959 }
08f37148b6a915a drivers/usb/gadget/function/f_fs.c Vincent Pelletier 2017-01-09 1960 }
a9e6f83c2df1991 drivers/usb/gadget/function/f_fs.c Michal Nazarewicz 2016-10-04 1961 spin_unlock_irqrestore(&func->ffs->eps_lock, flags);
ddf8abd2599491c drivers/usb/gadget/f_fs.c Michal Nazarewicz 2010-05-05 1962 }
ddf8abd2599491c drivers/usb/gadget/f_fs.c Michal Nazarewicz 2010-05-05 1963
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c index 3c584da..4edf339 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,21 +1937,24 @@ 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; + 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); + if (epfiles) { + epfiles->ep = NULL; + __ffs_epfile_read_buffer_free(epfiles); ++epfile; } }
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 <quic_ppratap@quicinc.com> Co-developed-by: Udipto Goswami <quic_ugoswami@quicinc.com> Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com> --- v3: Fixed the commit test description, removed the snippet for protecting read_buffer since epfile protection should be enough & renamed epfile to epfiles making it consistent with the field saving. drivers/usb/gadget/function/f_fs.c | 47 +++++++++++++++++++++++++++----------- 1 file changed, 34 insertions(+), 13 deletions(-)