[PATCH v2 0/2] dma-buf: dmabuf->file release-time cleanup

[Matt Evans <matt@ozlabs.org>]

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 [2/2] 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, the paired patch [1/2] correspondingly converts vmwgfx to
use get_file_active() too.

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

"v1" was the single "dma-buf: Annul dmabuf->file on file release"
patch:

 https://lore.kernel.org/all/f8efaabd-c06e-4f04-8cfa-489c148e37ba@ozlabs.org/

Matt Evans (2):
  drm/vmwgfx: Use get_file_active() for dmabuf->file
  dma-buf: Annul dmabuf->file on file release

 drivers/dma-buf/dma-buf.c           | 21 ++++++++++++++++++---
 drivers/gpu/drm/vmwgfx/ttm_object.c |  2 +-
 include/linux/dma-buf.h             |  4 +++-
 3 files changed, 22 insertions(+), 5 deletions(-)

-- 
2.47.3