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

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

[Yu Zhang]

ffs_dmabuf_detach() and ffs_dmabuf_find_attachment() dereference
ffs->gadget->dev.parent as their very first statement, without checking
that ffs->gadget is non-NULL:

	static int ffs_dmabuf_detach(struct file *file, int fd)
	{
		...
		struct device *dev = ffs->gadget->dev.parent;

ffs->gadget is only set between functionfs_bind() and
functionfs_unbind() (UDC bind/unbind). The epfile ioctls, however, are
gated only on ffs->state == FFS_ACTIVE, which userspace reaches by
writing the descriptors and strings and which is independent of the
gadget lifetime: an FFS instance is FFS_ACTIVE before it has ever been
bound to a UDC, and stays FFS_ACTIVE after unbind. So
FUNCTIONFS_DMABUF_DETACH (and the DMABUF transfer path, through
ffs_dmabuf_find_attachment()) can be issued while ffs->gadget is NULL,
dereferencing NULL:

	BUG: kernel NULL pointer dereference, address: 00000000000000b0
	RIP: 0010:ffs_epfile_ioctl+0x402/0x1700
	 __x64_sys_ioctl+0x120/0x170
	 do_syscall_64+0xf9/0x540

Reproduced by mounting functionfs, writing a descriptor+string blob so
the state becomes FFS_ACTIVE without ever binding a UDC, then issuing
FUNCTIONFS_DMABUF_DETACH on an endpoint file.

ffs_dmabuf_attach() already guards this with
"if (!gadget || !gadget->sg_supported) return -EPERM". Do the same in
ffs_dmabuf_detach() and ffs_dmabuf_find_attachment() (the latter also
covers ffs_dmabuf_transfer()): snapshot ffs->gadget, return -EPERM if it
is NULL, and use the snapshot.

Fixes: 7b07a2a7ca02 ("usb: gadget: functionfs: Add DMABUF import interface")
Cc: stable@vger.kernel.org
Signed-off-by: Yu Zhang <yuz08559@gmail.com>
---
 drivers/usb/gadget/function/f_fs.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 44218be..0976db7 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -1473,9 +1473,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);
 
@@ -1570,11 +1576,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);
-- 
2.43.0

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

[sashiko-bot]

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