linux-kernel - Re: [PATCH] uacce: fix concurrency of fops_open and uacce

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 20 Jun 2022 15:36:16 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Jean-Philippe Brucker <jean-philippe@...aro.org>
Cc:     Zhangfei Gao <zhangfei.gao@...aro.org>,
        Arnd Bergmann <arnd@...db.de>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Wangzhou <wangzhou1@...ilicon.com>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        linux-accelerators@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
        linux-crypto@...r.kernel.org, iommu@...ts.linux-foundation.org,
        Yang Shen <shenyang39@...wei.com>
Subject: Re: [PATCH] uacce: fix concurrency of fops_open and uacce_remove

On Mon, Jun 20, 2022 at 02:24:31PM +0100, Jean-Philippe Brucker wrote:
> On Fri, Jun 17, 2022 at 02:05:21PM +0800, Zhangfei Gao wrote:
> > > The refcount only ensures that the uacce_device object is not freed as
> > > long as there are open fds. But uacce_remove() can run while there are
> > > open fds, or fds in the process of being opened. And atfer uacce_remove()
> > > runs, the uacce_device object still exists but is mostly unusable. For
> > > example once the module is freed, uacce->ops is not valid anymore. But
> > > currently uacce_fops_open() may dereference the ops in this case:
> > > 
> > > 	uacce_fops_open()
> > > 	 if (!uacce->parent->driver)
> > > 	 /* Still valid, keep going */		
> > > 	 ...					rmmod
> > > 						 uacce_remove()
> > > 	 ...					 free_module()
> > > 	 uacce->ops->get_queue() /* BUG */
> > 
> > uacce_remove should wait for uacce->queues_lock, until fops_open release the
> > lock.
> > If open happen just after the uacce_remove: unlock, uacce_bind_queue in open
> > should fail.
> 
> Ah yes sorry, I lost sight of what this patch was adding. But we could
> have the same issue with the patch, just in a different order, no?
> 
> 	uacce_fops_open()
> 	 uacce = xa_load()
> 	 ...					rmmod

Um, how is rmmod called if the file descriptor is open?

That should not be possible if the owner of the file descriptor is
properly set.  Please fix that up.

thanks,

greg k-h