Hi all,
A fix for DMABUF's file-release path, to set dmabuf->file = NULL when
the struct file itself is being freed and avoid a stale pointer
lingering. Previously it could be assumed that dmabuf->file was
always valid until .release was called, but commit 4ab59c3c638c6
("dma-buf: Move dma_buf_release() from fops to dentry_ops") separated
dentry release (calls .release) from file release (can happen before
.release). Setting the pointer to NULL clearly delimits the period in
which it points to a valid struct file.
Raw dmabuf->file access is bad news unless the file is known to have a
reference held (for example, during export). A single-threaded
problem scenario is a dentry held open (e.g. using O_PATH) but the
file closed; .release is not called, so the exporter was unaware that
dmabuf->file became stale. Exporter activities using dmabuf->file
then led to a UAF. Or, threads racing concurrent access with a file
closing.
VFIO uses get_file_active(&dmabuf->file), which is a UAF when the
pointer goes stale. Alex Mastro has created a VFIO-centric reproducer
[1], which shows up with KASAN.
To fix, dmabuf->file is either valid or NULL. get_file_active() is
then safe and a recommended way to access dmabuf->file without an
existing reference. The only possible outcomes are for it to get a
reference to the file, to observe a valid pointer but the file in the
DEAD state (being closed, fail), or NULL (fail).
Patch [3/3] sets the pointer NULL when the file closes, which
get_file_active() handles correctly.
vmwgfx was identified as doing a problematic dmabuf->file dereference.
Although already a UAF, making dmabuf->file = NULL turns it into an
oops. So, patch [1/3] correspondingly converts vmwgfx to use
get_file_active() too.
i915 GVT unsafely dereferences dmabuf->file for a trivial debug
printout after the file reference moves to the dmabuf's fd, so patch
[2/3] dereferences in the safe window.
Added comments to highlight that dmabuf->file could go away before
.release, and to use get_file_active() if a reference isn't otherwise
already held.
Thanks,
Matt
[1]:
https://github.com/opsound/vfio-dmabuf-lab/blob/main/tests/vfio_dmabuf_opath_uaf_test.c
Changes:
v3:
- Better comment for struct dma_buf's file (refer to synchronisation
against ->release).
- Added trivial i915 patch to remove unsafe debug dmabuf->file accesss.
v2: https://lore.kernel.org/all/[email protected]/
"v1" was the single "dma-buf: Annul dmabuf->file on file release"
patch:
https://lore.kernel.org/all/[email protected]/
Matt Evans (3):
drm/vmwgfx: Use get_file_active() for dmabuf->file
drm/i915/gvt: Read dmabuf->file only while valid
dma-buf: Annul dmabuf->file on file release
drivers/dma-buf/dma-buf.c | 21 ++++++++++++++++++---
drivers/gpu/drm/i915/gvt/dmabuf.c | 5 +++--
drivers/gpu/drm/vmwgfx/ttm_object.c | 2 +-
include/linux/dma-buf.h | 5 ++++-
4 files changed, 26 insertions(+), 7 deletions(-)
--
2.47.3
