On 10/7/26 16:29, Matt Evans wrote:
> Hi Christian,
>
> On 07/10/2026 08:51, Christian König wrote:
>> Hi Matt,
>>
>> On 10/6/26 21:15, Matt Evans wrote:
>>> 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.
>>
>> Oh well, really good catch. And that problem is wider than this.
>>> Fixes: 4ab59c3c638c6 ("dma-buf: Move dma_buf_release() from fops to
>>> dentry_ops")
>>> Signed-off-by: Matt Evans <[email protected]>
>>
>> We seriously need a CC: stable here.
>
> I knew I forgot something :(
> >> ---
>>>
>>> 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.
>>
>> *sigh* we had multiple occasions suggesting to add a
>> dma_buf_get_rcu()/active() because exporters wanted to keep their DMA-buf on
>> linked lists or other container structures.
>>
>> That was always rejected because the DMA-buf framework had never guaranteed
>> that such things work correctly exactly because of those lifetime issues
>> here. In other words nobody thought that through and document what should
>> happen when etc...
>>
>> So the use cases in vmgfx and i915 are just the tip of the iceberg :)
>
> Yay :)
>
>> My suggestion is to get this patch reviewed and backported first and then
>> come up with a dma_buf_get_active() function including documentation and
>> comments and test case to make sure that this feature works *and* keeps
>> working.
>>
>> One minor comment below, apart from that the patch itself looks good to me.
>
> This makes sense to me, thank you for the context also.
>
> Just to make sure I thoroughly understand, do you envisage a
> dma_buf_get_active() to take both a reference on a dmabuf and its
> corresponding dmabuf->file so such users can move to a pattern of get-
> test-use-put on the file portion too? Sorry if it's a silly question,
> I'm not sure what to search for.
Yeah that won't really work. The dma_buf object uses the file to handles it's
lifetime and doesn't have a separate refcount.
I'm also not even sure if adding a dma_buf_get_active() is a good idea or not.
The problem with dma_buf_get_active() is still that it only works in the
exporter *if* there is a common lock hold while calling dma_buf_get_active()
and the cleanup inside the ->release callback.
That is totally specific to each exporter and can't be used by importers at
all, making stuff like that available to everybody is usually a recipe for
trouble.
Maybe it is better to just properly document on the dma_buf->file member that
exporters can use get_file_active(&dma_buf->file) but need to keep a bunch of
things in mind.
See below for some more thoughts.
>>> 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;
>>
>> I would do the casting only after the check.
>>
>> On the other hand this function is a callback of the file_operations so
>> checking if file->ops is really pointing to the file operations is
>> completely superfluous.
>>
>> So if you want to remove the check instead I'm perfectly fine with that as
>> well.
>
> I had given the unfamiliar code the benefit of the doubt, but this did
> look a bit suspect! Will do, thanks.
>
>
> Matt
>
> PS: I should've mentioned that Alex Mastro has kindly built a reproducer
> using a VFIO DMABUF export and dentry-held-open scenario. KASAN will
> detect the UAF:
>
>
> https://github.com/opsound/vfio-dmabuf-lab/blob/main/tests/vfio_dmabuf_opath_uaf_test.c
>
>>
>> Regards,
>> Christian.
>>
>>>
>>> - __dma_buf_list_del(file->private_data);
>>> + __dma_buf_list_del(dmabuf);
>>>
>>> + /* Must be observed by __get_file_rcu() before file_free() */
This here needs more meat and really explain the "why we need it" part.
Regards,
Christian.
>>> + smp_store_mb(dmabuf->file, NULL);
>>> return 0;
>>> }
>>>
>>
>
