The dmabuf release path is split between file and dentry release:
dma_buf_file_release() is called shortly before the file is freed, and
dma_buf_release() calls an exporter's dmabuf->ops->release when the
dentry is freed. However, the dentry can outlive the file (for
example, if opened with O_PATH), meaning .release might be called some
time after the file is freed.
This presents a window, when closing a DMABUF file, in which
dmabuf->file points to freed memory yet .release op has not yet been
called.
For VFIO, if a buffer's .release has not been called it's considered
still active and subject to move/cleanup. If so, it attempts to
get_file_active() on dmabuf->file: a file close with dentry held open
will call this function with a stale pointer, a UAF.
To make this pattern safe, set dmabuf->file to NULL in
dma_buf_file_release() to reflect that the associated file is now dead
even if the DMABUF is not yet gone. A get_file_active() ... fput()
sequence concurrent with a file close will only execute as one of:
- Gets the file before file_ref_put() (dmabuf->file valid)
- Observes dmabuf->file pointing to a file, but it's DEAD (no file)
- Observes dmabuf->file = NULL (no file)
Originally, drivers could assume dmabuf->file was valid until .release
was called from fops->release. This assumption was no longer valid
after 4ab59c3c638c6 ("dma-buf: Move dma_buf_release() from fops to
dentry_ops"), which moved the callback to the dentry release (by which
point the file might have been freed). With this commit, drivers must
still consider that dmabuf->file could be NULL before .release.
Fixes: 4ab59c3c638c6 ("dma-buf: Move dma_buf_release() from fops to dentry_ops")
Signed-off-by: Matt Evans <matt(a)ozlabs.org>
---
Hi,
This issue was found (by Claude Opus 5.5) in the context of VFIO's
DMABUF export path. VFIO iterates live DMABUFs with a
get_file_active()/fput() block, which now becomes safe if the file is
closed (and memory freed!) yet DMABUF .release hasn't yet occurred.
However, there are a couple of other places that directly use
dmabuf->file and seem able to race a closing file (i.e. without holding
the file reference)? If this is so, they'd be a UAF today; with this
patch that goes away, but instead of a stale pointer dmabuf->file could
be NULL:
1. drivers/gpu/drm/vmwgfx/ttm_object.c:get_dma_buf_unless_doomed()
file_ref_get(&dmabuf->file->f_ref); on a non-refcounted DMABUF
The commit message of 90ee6ed776c0 ("fs: port files to file_ref") hints
this might be more subtle than replacing it with a get_file() variant
(so as to accept a NULL file *).
2. drivers/gpu/drm/i915/gvt/dmabuf.c:intel_vgpu_get_dmabuf()
gvt_dbg_dpy(... file_count(dmabuf->file) ...);
Respective vmwgfx & i915 maintainers, what is your view?
Or, indeed, if anyone sees any other questionable uses of dmabuf->file.
Thanks,
Matt
drivers/dma-buf/dma-buf.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 4c9add51f9ef..726639130477 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -193,11 +193,15 @@ static void dma_buf_release(struct dentry *dentry)
static int dma_buf_file_release(struct inode *inode, struct file *file)
{
+ struct dma_buf *dmabuf = file->private_data;
+
if (!is_dma_buf_file(file))
return -EINVAL;
- __dma_buf_list_del(file->private_data);
+ __dma_buf_list_del(dmabuf);
+ /* Must be observed by __get_file_rcu() before file_free() */
+ smp_store_mb(dmabuf->file, NULL);
return 0;
}
--
2.47.3
On 10/6/26 02:48, Val Packett wrote:
> Shared memory buffers for software-rendered GUI applications are often
> large and highly fragmented in memory. To get a shareable handle to such
> a buffer from a guest VM, a virtio-gpu device backend can use the
> UDMABUF_CREATE_LIST ioctl with the memfds backing guest RAM and the list
> of guest physical pages sent by the guest driver. That list reaches 80K
> entries for a maximized window on a 4K display!
>
> Like 44e9eb5a7621 ("dma-buf/udmabuf: Disable the size limit by default")
> did for the size limit, change the default to INT_MAX since the amount
> of protection provided by this limit seems dubious.
>
> Signed-off-by: Val Packett <val(a)invisiblethingslab.com>
> ---
> drivers/dma-buf/udmabuf.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index df6dd0046242..589031133588 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -16,9 +16,9 @@
> #include <linux/vmalloc.h>
> #include <linux/iosys-map.h>
>
> -static int list_limit = 1024;
> +static int list_limit = INT_MAX;
> module_param(list_limit, int, 0644);
> -MODULE_PARM_DESC(list_limit, "udmabuf_create_list->count limit. Default is 1024.");
> +MODULE_PARM_DESC(list_limit, "udmabuf_create_list->count limit. Default is INT_MAX.");
This will completely overflow the size calculation in udmabuf_ioctl_create_list().
Please fix udmabuf_ioctl_create_list() to use memdup_array_user() and use a reasonable limit here. Something like 128k should probably do.
Apart from that I think we need to start using huge and giant pages for virtio-gpu on the client side, we already had it multiple times that we hit limits with that in multiple places.
Sharing 80k individual 4k pointers is really not very efficient.
Regards,
Christian.
>
> static int size_limit_mb = INT_MAX;
> module_param(size_limit_mb, int, 0644);
On 10/5/26 11:55, Fred Griffoul wrote:
> From: Fred Griffoul <fgriffo(a)amazon.co.uk>
>
> dma_buf_invalidate_mappings() tells every importer that the whole
> buffer changed. An exporter that changes one part of its memory cannot
> say which bytes changed, so importers throw away mappings that are
> still valid.
>
> Add an exporter helper that invalidates a byte range, and an importer
> callback that receives it. The callback means that the address, the
> attributes or the backing of the range changed. If part of the range is
> no longer backed, get_phys() returns -ENOENT for it. Importers must stop
> using their old answer before the callback returns. Importers that do
> not implement the callback still receive a whole-buffer invalidation.
Yeah that is exactly one of the reasons why we don't allow that.
Clear NAK to the whole approach. See the reply to patch #2 for a detailed description.
Regards,
Christian.
>
> Signed-off-by: Fred Griffoul <fgriffo(a)amazon.co.uk>
> ---
> drivers/dma-buf/dma-buf.c | 30 ++++++++++++++++++++++++++++++
> include/linux/dma-buf.h | 16 ++++++++++++++++
> 2 files changed, 46 insertions(+)
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 66b85d53ed22..e7010163eb2f 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -1389,6 +1389,36 @@ void dma_buf_invalidate_mappings(struct dma_buf *dmabuf)
> }
> EXPORT_SYMBOL_NS_GPL(dma_buf_invalidate_mappings, "DMA_BUF");
>
> +/**
> + * dma_buf_invalidate_mappings_range - notify attachments that a range changed
> + * @dmabuf: buffer whose layout changed
> + * @offset: first changed byte
> + * @length: number of changed bytes
> + *
> + * Importers with a ranged callback stop using their old mappings of the range
> + * before returning. Other importers receive the existing whole-buffer
> + * callback, which is correct but coarser. The reservation lock must be held.
> + */
> +void dma_buf_invalidate_mappings_range(struct dma_buf *dmabuf,
> + unsigned long offset,
> + unsigned long length)
> +{
> + struct dma_buf_attachment *attach;
> +
> + dma_resv_assert_held(dmabuf->resv);
> + list_for_each_entry(attach, &dmabuf->attachments, node) {
> + const struct dma_buf_attach_ops *ops = attach->importer_ops;
> +
> + if (!ops)
> + continue;
> + if (ops->invalidate_mappings_range)
> + ops->invalidate_mappings_range(attach, offset, length);
> + else if (ops->invalidate_mappings)
> + ops->invalidate_mappings(attach);
> + }
> +}
> +EXPORT_SYMBOL_NS_GPL(dma_buf_invalidate_mappings_range, "DMA_BUF");
> +
> /**
> * dma_buf_get_phys - describe the run that starts at an offset
> * @attach: attachment to query
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> index b223962e20c2..55c3fe60a0ba 100644
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -485,6 +485,19 @@ struct dma_buf_attach_ops {
> * required behavior.
> */
> void (*invalidate_mappings)(struct dma_buf_attachment *attach);
> +
> + /**
> + * @invalidate_mappings_range: [optional] a byte range changed
> + *
> + * The exporter changed the address, attributes or backing of
> + * [@offset, @offset + @length). The importer must stop using its old
> + * answer for that range before returning.
> + * Importers without this callback receive @invalidate_mappings for
> + * the whole buffer instead.
> + */
> + void (*invalidate_mappings_range)(struct dma_buf_attachment *attach,
> + unsigned long offset,
> + unsigned long length);
> };
>
> /**
> @@ -600,6 +613,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *,
> void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *,
> enum dma_data_direction);
> void dma_buf_invalidate_mappings(struct dma_buf *dma_buf);
> +void dma_buf_invalidate_mappings_range(struct dma_buf *dma_buf,
> + unsigned long offset,
> + unsigned long length);
> bool dma_buf_attach_revocable(struct dma_buf_attachment *attach);
> /* bits 0-7: memory type (a value, not flags) */
> #define DMA_BUF_PHYS_ATTR_TYPE_MASK GENMASK(7, 0)
> --
> 2.47.3
>
On Mon, Sep 28, 2026 at 02:32:22PM +0100, Pavel Begunkov wrote:
> +static void nvme_dmabuf_map_sync_for_cpu(struct nvme_dev *nvme_dev,
> + struct request *req)
> +{
> + struct device *dev = nvme_dev->dev;
> + enum dma_data_direction dma_dir;
> + struct bio *bio = req->bio;
> + struct nvme_dmabuf_map *map = to_nvme_dmabuf_map(bio->bi_dmabuf_map);
> +
> + dma_dir = rq_data_dir(req) == READ ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
> + dma_sync_sgtable_for_cpu(dev, map->sgt, dma_dir);
This can use rq_dma_dir in the argument. Same for the other DMA API
calls.
Btw, given how larger the dmabuf code is now, have you considered
splitting it into a new nvme-pci-dmabuf.c file?
On Mon, Sep 28, 2026 at 02:32:21PM +0100, Pavel Begunkov wrote:
> Add a simple proxy implementation of init_dma_buf_io_ctx() forwarding
> the call to a new struct block_device_operations operation. Also reject
> dma-buf backed iterators for buffered IO.
>
> Reviewed-by: Christoph Hellwig <hch(a)lst.de>
> [pavel: reject dma-buf without O_DIRECT]
> Signed-off-by: Pavel Begunkov <asml.silence(a)gmail.com>
The Sashiko report about the errno on fallback here looks real,
and is probably easily fixed by setting a proper errno.
On Mon, Sep 28, 2026 at 02:32:18PM +0100, Pavel Begunkov wrote:
> Introduce a new iterator type for dmabuf maps. The map in an opaque
> object with internals and format specific to the subsystem / driver, and
> only it can use that subsystem / driver for issuing IO. The task of the
> middle layers is to pass the map / iterator further down, maybe doing
> basic splitting and length checking. The iterator can only be used by
> operations of the file the associated map was created for.
>
> Suggested-by: Keith Busch <kbusch(a)kernel.org>
> Reviewed-by: Christoph Hellwig <hch(a)lst.de>
> Signed-off-by: Pavel Begunkov <asml.silence(a)gmail.com>
FYI, I think the Sashiko complaint about iterate_and_advance2
is correct here. We don't really hit that in the series, but leaving
iov_iter methods partially implemented and fall through to others
seems dangerous. At minimum that needs a WARN_ON to catch that case.