Re: [PATCH] usb: gadget: f_fs: check ffs->gadget in the DMABUF detach/find paths

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: gadget: f_fs: unsynchronized ffs->gadget access in DMABUF paths
--

commit 11858230a366468ebcd956f6817d9428b7b67ebc
Author: Yu Zhang <yuz08559@gmail.com>

usb: gadget: f_fs: check ffs->gadget in the DMABUF detach/find paths

This commit adds NULL checks on ffs->gadget in ffs_dmabuf_find_attachment()
and ffs_dmabuf_detach() to prevent a NULL pointer dereference when endpoint
DMABUF ioctls are issued while the functionfs instance is not bound to a UDC.

> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index c64a268e98a43..52d486ec218a9 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -1507,9 +1507,15 @@ static int ffs_dma_resv_lock(struct dma_buf *dmabuf, bool nonblock)
>  static struct dma_buf_attachment *
>  ffs_dmabuf_find_attachment(struct ffs_epfile *epfile, struct dma_buf *dmabuf)
>  {
> -	struct device *dev = epfile->ffs->gadget->dev.parent;
> +	struct usb_gadget *gadget = epfile->ffs->gadget;
>  	struct dma_buf_attachment *attach = NULL;
>  	struct ffs_dmabuf_priv *priv;
> +	struct device *dev;
> +
> +	if (!gadget)
> +		return ERR_PTR(-EPERM);
> +
> +	dev = gadget->dev.parent;
>  
>  	mutex_lock(&epfile->dmabufs_mutex);
>  
> @@ -1604,11 +1610,17 @@ static int ffs_dmabuf_detach(struct file *file, int fd)
>  {
>  	struct ffs_epfile *epfile = file->private_data;
>  	struct ffs_data *ffs = epfile->ffs;
> -	struct device *dev = ffs->gadget->dev.parent;
> +	struct usb_gadget *gadget = ffs->gadget;
>  	struct ffs_dmabuf_priv *priv, *tmp;
>  	struct dma_buf *dmabuf;
> +	struct device *dev;
>  	int ret = -EPERM;
>  
> +	if (!gadget)
> +		return -EPERM;
> +
> +	dev = gadget->dev.parent;
> +
>  	dmabuf = dma_buf_get(fd);
>  	if (IS_ERR(dmabuf))
>  		return PTR_ERR(dmabuf);

[Severity: High]
Can reading ffs->gadget without holding ffs->mutex still race with a
concurrent UDC unbind, and does returning -EPERM in ffs_dmabuf_detach()
prevent userspace from detaching DMABUFs after unbind?

In functionfs_unbind(), ffs->gadget is cleared under ffs->mutex:

drivers/usb/gadget/function/f_fs.c:functionfs_unbind() {
	...
	mutex_lock(&ffs->mutex);
	usb_ep_free_request(ffs->gadget->ep0, ffs->ep0req);
	ffs->ep0req = NULL;
	ffs->gadget = NULL;
	clear_bit(FFS_FL_BOUND, &ffs->flags);
	mutex_unlock(&ffs->mutex);
}

In both ffs_dmabuf_find_attachment() and ffs_dmabuf_detach(), ffs->gadget is
read into a local variable without holding ffs->mutex and without READ_ONCE().
Without a lock or compiler barrier, the compiler may reload ffs->gadget after
the NULL check, or functionfs_unbind() and usb_del_gadget_udc() may complete
and free struct usb_gadget via usb_put_gadget() before gadget->dev.parent is
dereferenced.

Additionally, ffs_func_unbind() does not remove attached DMABUFs from
epfile->dmabufs when the UDC is unbound. Once functionfs_unbind() sets
ffs->gadget to NULL, returning -EPERM when !gadget in ffs_dmabuf_detach()
blocks userspace from detaching previously attached DMABUFs via
FUNCTIONFS_DMABUF_DETACH after receiving FUNCTIONFS_UNBIND (unlike
ffs_epfile_release(), which detaches entries from epfile->dmabufs without
checking ffs->gadget).

Could ffs_dmabuf_detach() match only on priv->attach->dmabuf == dmabuf
without consulting ffs->gadget, and could ffs_dmabuf_find_attachment()
synchronize against functionfs_unbind()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020051925.92294-1-yuz08559@gmail.com?part=1