From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: dma-buf: add DMA_BUF_IOCTL_DERIVE for reduced-permission aliases
Date: Mon, 25 May 2026 20:27:33 +1000
Message-ID: <review-patch1-20260521-dmabuf-limit-access-v1-1-26c01e27365a@redhat.com>
In-Reply-To: <20260521-dmabuf-limit-access-v1-1-26c01e27365a@redhat.com>
References: <20260521-dmabuf-limit-access-v1-0-26c01e27365a@redhat.com> <20260521-dmabuf-limit-access-v1-1-26c01e27365a@redhat.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Bug (security): O_WRONLY escalation not blocked**

The escalation check only blocks `O_RDWR`:

```c
/* Escalating permissions is not allowed. */
if ((params.flags & O_ACCMODE) =3D=3D O_RDWR &&
    !(file->f_mode & FMODE_WRITE))
    return -EACCES;
```

Since `O_RDONLY =3D 0`, `O_WRONLY =3D 1`, `O_RDWR =3D 2`: when `params.flag=
s & O_ACCMODE =3D=3D O_WRONLY` (1), the condition `(1) =3D=3D O_RDWR (2)` i=
s false, and the ioctl proceeds to create a write-capable fd from a read-on=
ly one. The mmap check added to `dma_buf_mmap_internal()` doesn't help here=
 =E2=80=94 the derived fd would have `FMODE_WRITE` set, passing the mmap ch=
eck too.

The fix should check that *any* write access is not being escalated:

```c
if ((params.flags & O_ACCMODE) !=3D O_RDONLY &&
    !(file->f_mode & FMODE_WRITE))
    return -EACCES;
```

For full correctness, the reverse should also be checked (deriving `O_RDONL=
Y` from an `O_WRONLY` fd would grant read access), though `O_WRONLY` dma-bu=
fs don't exist in practice:

```c
if ((params.flags & O_ACCMODE) !=3D O_WRONLY &&
    !(file->f_mode & FMODE_READ))
    return -EACCES;
```

**Minor: invalid access mode O_ACCMODE (3) not rejected**

The flags validation:

```c
if (params.flags & ~(O_ACCMODE | O_CLOEXEC))
    return -EINVAL;
```

permits the bit pattern `3` for the access mode. `OPEN_FMODE(3)` yields `(3=
+1) & 3 =3D 0`, creating a file with neither `FMODE_READ` nor `FMODE_WRITE`=
. While not a security issue, it's invalid input that should be rejected wi=
th `-EINVAL`.

**Design concern: DMA_BUF_IOCTL_SYNC not restricted on read-only fds**

`dma_buf_ioctl` dispatches `DMA_BUF_IOCTL_SYNC` on the `struct dma_buf` wit=
hout checking file access mode. A read-only fd can call `begin_cpu_access` =
/ `end_cpu_access` with `DMA_BUF_SYNC_WRITE` direction. This doesn't itself=
 enable writing, but it's inconsistent with the read-only intent and could =
trigger unnecessary cache operations.

**Design concern: kernel-side DMA access is unrestricted**

A kernel driver that receives the derived read-only fd via `dma_buf_get()` =
gets back the same `struct dma_buf` with no indication that it came from a =
reduced-permission fd. It can `dma_buf_attach` + `dma_buf_map_attachment(DM=
A_BIDIRECTIONAL)` and get write DMA access. This is probably acceptable for=
 v1 =E2=80=94 the trust boundary is between user-space components =E2=80=94=
 but worth documenting as a known limitation.

**Style nit: early private_data access**

```c
static int dma_buf_file_release(struct inode *inode, struct file *file)
{
	struct dma_buf *dmabuf =3D file->private_data;

	if (!is_dma_buf_file(file))
		return -EINVAL;

	if (file !=3D dmabuf->file)
		dma_buf_put(dmabuf);
```

`dmabuf` is read before `is_dma_buf_file()` validates the file. While harml=
ess (not dereferenced until after the check), moving the assignment after t=
he check would be clearer.

**Lifecycle correctness**

The refcounting logic is correct: `get_dma_buf()` on creation increments th=
e primary file's refcount, and `dma_buf_put()` on derived-file release decr=
ements it. The move of `__dma_buf_list_del` to `dma_buf_release()` (dentry =
destruction) is necessary and correct =E2=80=94 without it, closing the fir=
st derived fd would `list_del` the node, and closing a second would corrupt=
 memory.

**Forward declaration of dma_buf_fops**

```c
static const struct file_operations dma_buf_fops;
```

This is needed so `dma_buf_ioctl_derive` (above the definition) can referen=
ce it. It works, but an alternative would be to move `dma_buf_ioctl_derive`=
 below the definition to avoid the forward declaration. Minor style prefere=
nce.

---

---
Generated by Claude Code Patch Reviewer