[PATCH V1] accel/amdxdna: Fix user buffer VMA checking and page pinning locking

[PATCH V1] accel/amdxdna: Fix user buffer VMA checking and page pinning locking

[Lizhi Hou]

From: Max Zhen <max.zhen@amd.com>

Holding mmap_read_lock across user buffer VMA checking and page pinning.
This keeps VMA validation and user page pinning synchronized with mmap
updates.

Also adding the check for VM_MAYWRITE to make sure the mapping can't be
updated to writable after pinning of the pages.

Fixes: f649e63d4a64 ("accel/amdxdna: Support read-only user-pointer BO mappings")
Signed-off-by: Max Zhen <max.zhen@amd.com>
Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
---
 drivers/accel/amdxdna/amdxdna_ubuf.c | 34 ++++++++++++++++------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_ubuf.c b/drivers/accel/amdxdna/amdxdna_ubuf.c
index df4ab225fbf9..85c3d6f46845 100644
--- a/drivers/accel/amdxdna/amdxdna_ubuf.c
+++ b/drivers/accel/amdxdna/amdxdna_ubuf.c
@@ -99,17 +99,13 @@ static int readonly_va_entry(struct amdxdna_drm_va_entry *va_ent)
 	struct vm_area_struct *vma;
 	int ret;
 
-	mmap_read_lock(mm);
-
 	vma = find_vma(mm, va_ent->vaddr);
 	if (!vma ||
 	    vma->vm_start > va_ent->vaddr ||
 	    vma->vm_end - va_ent->vaddr < va_ent->len)
 		ret = -ENOENT;
 	else
-		ret = vma->vm_flags & VM_WRITE ? 0 : 1;
-
-	mmap_read_unlock(mm);
+		ret = vma->vm_flags & (VM_WRITE | VM_MAYWRITE) ? 0 : 1;
 	return ret;
 }
 
@@ -123,7 +119,7 @@ struct dma_buf *amdxdna_get_ubuf(struct drm_device *dev,
 	u32 npages, start = 0;
 	struct dma_buf *dbuf;
 	bool readonly = true;
-	int i, ret;
+	int i, ret = 0;
 	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
 
 	if (!can_do_mlock())
@@ -161,10 +157,6 @@ struct dma_buf *amdxdna_get_ubuf(struct drm_device *dev,
 			ret = -EINVAL;
 			goto free_ent;
 		}
-
-		/* Pin pages as writable as long as not all entries are read-only. */
-		if (readonly && readonly_va_entry(&va_ent[i]) != 1)
-			readonly = false;
 	}
 
 	ubuf->nr_pages = exp_info.size >> PAGE_SHIFT;
@@ -183,25 +175,37 @@ struct dma_buf *amdxdna_get_ubuf(struct drm_device *dev,
 		goto sub_pin_cnt;
 	}
 
+	mmap_read_lock(current->mm);
+
+	for (i = 0; i < num_entries; i++) {
+		/* Pin pages as writable as long as not all entries are read-only. */
+		if (readonly && readonly_va_entry(&va_ent[i]) != 1)
+			readonly = false;
+	}
 	for (i = 0; i < num_entries; i++) {
 		npages = va_ent[i].len >> PAGE_SHIFT;
 
-		ret = pin_user_pages_fast(va_ent[i].vaddr, npages,
-					  (readonly ? 0 : FOLL_WRITE) | FOLL_LONGTERM,
-					  &ubuf->pages[start]);
+		ret = pin_user_pages(va_ent[i].vaddr, npages,
+				     (readonly ? 0 : FOLL_WRITE) | FOLL_LONGTERM,
+				     &ubuf->pages[start]);
 		if (ret >= 0) {
 			start += ret;
 			if (ret != npages) {
 				XDNA_ERR(xdna, "Partially pinned pages %d/%u", ret, npages);
 				ret = -ENOMEM;
-				goto destroy_pages;
+				break;
 			}
 		} else {
 			XDNA_ERR(xdna, "Failed to pin pages ret %d", ret);
-			goto destroy_pages;
+			break;
 		}
 	}
 
+	mmap_read_unlock(current->mm);
+
+	if (ret < 0)
+		goto destroy_pages;
+
 	exp_info.ops = &amdxdna_ubuf_dmabuf_ops;
 	exp_info.priv = ubuf;
 	exp_info.flags = (readonly ? O_RDONLY : O_RDWR) | O_CLOEXEC;
-- 
2.34.1

Claude review: accel/amdxdna: Fix user buffer VMA checking and page pinning locking

[Claude Code Review Bot]

Overall Series Review

Subject: accel/amdxdna: Fix user buffer VMA checking and page pinning locking
Author: Lizhi Hou <lizhi.hou@amd.com>
Patches: 1
Reviewed: 2026-05-16T08:54:21.992526

---

This is a single-patch fix addressing a real **TOCTOU (time-of-check-time-of-use) vulnerability** in the amdxdna accelerator driver's user buffer handling. The core problem was that VMA permission checking (`readonly_va_entry`) and page pinning (`pin_user_pages_fast`) were done without a common lock, allowing an attacker to modify the VMA (via `mprotect`, `munmap`/`mmap`, etc.) between the check and the pin.

The fix is well-targeted and correct in its approach:
1. Holds `mmap_read_lock` across **both** the VMA permission check and the page pinning.
2. Switches from `pin_user_pages_fast` to `pin_user_pages`, which expects the caller to already hold `mmap_read_lock`.
3. Adds `VM_MAYWRITE` to the writability check to close a `mprotect`-based bypass.

The error paths are correctly restructured (goto → break, with unlock before cleanup jump). The locking semantics are consistent with the GUP API contract.

**One minor concern** exists (detailed below) but overall this is a sound security fix that should be accepted.

---

---
Generated by Claude Code Patch Reviewer

Claude review: accel/amdxdna: Fix user buffer VMA checking and page pinning locking

[Claude Code Review Bot]

Patch Review

**Positive aspects:**

1. **TOCTOU fix is correct.** The original code in `readonly_va_entry()` acquired and released `mmap_read_lock` independently:
   ```c
   // OLD: lock/unlock scoped only to the VMA check
   mmap_read_lock(mm);
   vma = find_vma(mm, va_ent->vaddr);
   ...
   ret = vma->vm_flags & VM_WRITE ? 0 : 1;
   mmap_read_unlock(mm);
   ```
   Then `pin_user_pages_fast()` was called later without the lock. The new code correctly lifts the lock to the caller (`amdxdna_get_ubuf`) and holds it across both the readonly determination loop and the page pinning loop:
   ```c
   mmap_read_lock(current->mm);
   
   for (i = 0; i < num_entries; i++) {
       if (readonly && readonly_va_entry(&va_ent[i]) != 1)
           readonly = false;
   }
   for (i = 0; i < num_entries; i++) {
       ...
       ret = pin_user_pages(va_ent[i].vaddr, npages,
                            (readonly ? 0 : FOLL_WRITE) | FOLL_LONGTERM,
                            &ubuf->pages[start]);
       ...
   }
   
   mmap_read_unlock(current->mm);
   ```

2. **`pin_user_pages_fast` → `pin_user_pages` switch is correct.** `pin_user_pages()` passes `NULL` for the `locked` parameter to `is_valid_gup_args()`, so `FOLL_UNLOCKABLE` is not set. This means the GUP internals assume mmap_read_lock is already held and won't try to manage it. The caller holds the lock — consistent.

3. **`VM_MAYWRITE` addition is important.** The old check:
   ```c
   ret = vma->vm_flags & VM_WRITE ? 0 : 1;
   ```
   only checked current writability. The new check:
   ```c
   ret = vma->vm_flags & (VM_WRITE | VM_MAYWRITE) ? 0 : 1;
   ```
   prevents the scenario where a user pins pages from a read-only VMA that has `VM_MAYWRITE`, then calls `mprotect()` to gain write access to the DMA-pinned pages. Without this, the read-only pinning could be bypassed after the fact.

4. **Error path restructuring is correct.** Changing `goto destroy_pages` to `break` inside the pinning loop, followed by `mmap_read_unlock()` and then the `if (ret < 0) goto destroy_pages` check, ensures the lock is always released before cleanup. The success case (all pages pinned) has `ret` set to the last `pin_user_pages()` return (positive), so the `ret < 0` check correctly skips the error path.

5. **`ret = 0` initialization** is necessary since `ret` is now checked after the loops, and in the edge case of `num_entries == 0`, the pinning loop body never executes.

**Minor concern:**

- **`readonly_va_entry()` error return silently treated as writable.** When `readonly_va_entry()` returns `-ENOENT` (VMA not found or doesn't cover the range), the check `!= 1` evaluates to true, so `readonly` is set to `false`. This means pages will be pinned with `FOLL_WRITE`. The `pin_user_pages()` call will ultimately fail because the VMA doesn't exist, so there's no security issue — but it would be cleaner to propagate the error early rather than relying on `pin_user_pages` to fail later. This also existed in the original code, so it's not a regression. Consider:
  ```c
  for (i = 0; i < num_entries; i++) {
      ret = readonly_va_entry(&va_ent[i]);
      if (ret < 0)
          break;
      if (readonly && ret != 1)
          readonly = false;
  }
  if (ret < 0) {
      mmap_read_unlock(current->mm);
      goto destroy_pages;
  }
  ```
  This is a suggestion for improvement, not a blocker.

**No issues found with:**
- Lock ordering (mmap_read_lock is a reader lock; no nesting conflicts with GUP internals since FOLL_UNLOCKABLE is not set)
- The `static` function `readonly_va_entry()` being called only from the locked region (confirmed: single call site at line 214)
- Consistency between `current->mm` and `ubuf->mm` (set to `current->mm` at line 164)

---
Generated by Claude Code Patch Reviewer