Re: [PATCH] dma-buf: Annul dmabuf->file on file release

[Matt Evans <matt@ozlabs.org>]

Hi i915 folks,

On 08/10/2026 14:41, Matt Evans wrote:
> Hi Zack,
> 
> On 07/10/2026 16:24, Zack Rusin wrote:
>> On Tue, Oct 6, 2026 at 3:15 PM Matt Evans <matt@ozlabs.org> 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.
>>>
>>> Fixes: 4ab59c3c638c6 ("dma-buf: Move dma_buf_release() from fops to 
>>> dentry_ops")
>>> Signed-off-by: Matt Evans <matt@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
>>
>> Yes, that's a real UAF today. vmwgfx relied on the exporter's
>> dma_buf_ops.release running inside __fput() before file_free(), which
>> stopped being true with 4ab59c3c638c and we missed it.
>>
>> But this patch on its own makes vmwgfx worse. In the ordinary case,
>> where closing the file drops the last dentry reference, a close racing
>> PRIME_HANDLE_TO_FD is safe today: ttm_prime_dmabuf_release() runs from
>> the same __fput() and blocks on prime->mutex, keeping the file
>> allocated, so file_ref_get() just fails on the dead refcount. With
>> this patch dmabuf->file is NULL in that window and we oops. So this
>> shouldn't go to stable without a vmwgfx fix.
>>
>> For vmwgfx we'd probably need this one line change in
>> drivers/gpu/drm/vmwgfx/ttm_object.c to accompany this patch:
>> @@ -471,7 +475,7 @@ void ttm_object_device_release(struct
>> ttm_object_device **p_tdev)
>>    */
>>   static bool __must_check get_dma_buf_unless_doomed(struct dma_buf 
>> *dmabuf)
>>   {
>> -     return file_ref_get(&dmabuf->file->f_ref);
>> +     return get_file_active(&dmabuf->file) != NULL;
>>   }
>>
>> under prime->mutex, which keeps the struct dma_buf alive. It needs the
>> NULL store from this patch to fix the existing UAF, since otherwise
>> the recheck can accept a recycled file through the unchanged stale
>> pointer.
> 
> Thanks for confirming!  Ah, so get_file_active() is good here, cheers. 
> I'll repost as a 2-part series to first include this in vmwgfx and 
> second add the anNULL.
> 
>>> 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) ...);
> 
> FWIW aside, I re-read this, and there does appear to be a ref held 
> (dma_buf_export() has just happened, and dma_buf_fd() generated) so 
> dmabuf->file is stable at that point.

I think I was mistaken here:  the reference is moved to the file table 
by dma_buf_fd(), and seems another thread could immediately close it.

If so, there's an opportunity for userspace to (after this fix) cause a 
NULL deref here.

I'm about to repost the series but am happy to include a oneliner third 
patch to mitigate this, if you feel appropriate.

Is file_count() still interesting here, i.e. do you expect it to be >1 
immediately after export?  If still required, reading 
file_count(dmabuf->file) before the dma_buf_fd() would be safe (printing 
it later at the existing place).


Matt

> 
>>> 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);
>>
>> I think Alex is right and dmabuf can be NULL here. An early if
>> (!dmabuf) return 0" will fix that.
> 
> Yep, on it.
> 
> Many thanks,
> 
> 
> Matt
>