Re: [PATCH RFC 11/13] drm/powervr_sgx: Validate shader code in the kernel

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential
issues to consider:
- [High] drm/powervr_sgx: partial code BO mappings bypass branch and
  phase checks
- [High] drm/powervr_sgx: torn GPU instruction fetch in
  pvrsgx_ioctl_upload_code() at offset >= code_end
- [High] drm/powervr_sgx: missing cache maintenance in
  pvrsgx_ioctl_upload_code()
- [Medium] drm/powervr_sgx: code_place_mapped() ignores earlier invalid
  mappings
- [Medium] drm/powervr_sgx: monotonic code_end and code_abs in
  pvrsgx_ioctl_upload_code()
- [Medium] drm/powervr_sgx: code_place_mapped() ignores unmapping BOs in
  UPLOAD_CODE
- [Low] drm/powervr_sgx: spurious -EBUSY in pvrsgx_ioctl_upload_code()
  during VM_BIND error cleanup
--

commit 3c08e583541e226f931172772a47e077e959d3ed
Author: Joseph Telaak <joseph@telaak.dev>

drm/powervr_sgx: Validate shader code in the kernel

This commit adds USSE shader instruction validation, code object mapping
restrictions, and the UPLOAD_CODE ioctl for PowerVR SGX GPUs.

> diff --git a/drivers/gpu/drm/powervr_sgx/pvrsgx_validate.c b/drivers/gpu/drm/powervr_sgx/pvrsgx_validate.c
> new file mode 100644
> index 0000000000000..cf9cfcf87fdac
> --- /dev/null
> +++ b/drivers/gpu/drm/powervr_sgx/pvrsgx_validate.c

[ ... ]

> +/* Code objects, and only they, go into the USSE heaps, read-only. */
> +int pvrsgx_validate_vm_map(struct pvrsgx_device *sdev, struct pvrsgx_vm *vm,
> +			   unsigned int heap, struct pvrsgx_bo *bo, u64 offset, u64 size)
> +{
> +	return heap_is_usse(heap) == bo_is_code(bo) ? 0 : -EINVAL;
> +}
> +
> +static_assert(PVRSGX_USSE_WINDOW_INSTS * 8 == 1u << PVRSGX_USSE_WINDOW_LOG2);
> +
> +/*
> + * Where a mapping at addr of the object from offset puts the object's start,
> + * in instructions from the start of its code base window: what phas.i's
> + * targets count from when the task's PDS program names the window its first
> + * word is in. One window per object makes that base the same for every entry.
> + */
> +static s64 code_place(const struct pvrsgx_heap_layout *h, u64 addr, u64 offset)
> +{
> +	u64 base = addr >= h->usse_vertex.base ? h->usse_vertex.base : h->usse_pixel.base;
> +	s64 start = (s64)(addr - base) - (s64)offset;
> +
> +	if (start < 0)
> +		return PVRSGX_USSE_PLACE_NONE;
> +	return (start / 8) & (PVRSGX_USSE_WINDOW_INSTS - 1);
> +}

[Severity: High]
Can a partial mapping of a code object let br or phas.i jump outside the
mapped range and the USSE heap?

pvrsgx_ioctl_upload_code() passes obj->size / 8 to pvrsgx_usse_scan(),
which checks br targets against [0, obj_insts) and phas.i targets against
[place, place + obj_insts).  However, pvrsgx_validate_vm_map() ignores
offset and size, and DRM_POWERVR_SGX_VM_BIND_OP_UNMAP can also split or
partially unmap an existing code mapping.

If userspace creates a 1 MiB + 4 KiB code BO, uploads a br at offset
1 MiB with displacement -131072 instructions (target 0), and maps only
the last 4 KiB page at h->usse_pixel.base, executing that instruction
branches to h->usse_pixel.base - 1 MiB (which lands in kernel_data on
single-context SGX530 with allow_unisolated=1).  For phas.i, partial
mappings likewise let targets in [0, obj_insts) jump past the mapped
subrange, and code_place() wraps modulo 512 KiB when addr and
addr - offset fall in different windows.

> +/*
> + * Once code with absolute targets is in an object, every mapping of it puts it
> + * where those targets were checked. Called with the object's reservation held.
> + */
> +int pvrsgx_validate_vm_link(struct pvrsgx_bo *bo, u64 addr, u64 offset)
> +{
> +	if (!bo_is_code(bo) || !bo->code_abs)
> +		return 0;
> +	const struct pvrsgx_heap_layout *h = pvrsgx_heaps(to_pvrsgx_device(bo->base.base.dev));
> +
> +	return code_place(h, addr, offset) == bo->code_place ? 0 : -EINVAL;
> +}
> +
> +/* The one place every mapping of obj puts it, or PVRSGX_USSE_PLACE_NONE. */
> +static s64 code_place_mapped(struct drm_gem_object *obj)
> +{
> +	const struct pvrsgx_heap_layout *h = pvrsgx_heaps(to_pvrsgx_device(obj->dev));
> +	struct drm_gpuvm_bo *vm_bo;
> +	struct drm_gpuva *va;
> +	s64 place = PVRSGX_USSE_PLACE_NONE;
> +
> +	drm_gem_for_each_gpuvm_bo(vm_bo, obj) {
> +		drm_gpuvm_bo_for_each_va(va, vm_bo) {
> +			s64 p = code_place(h, va->va.addr, va->gem.offset);
> +
> +			if (place != PVRSGX_USSE_PLACE_NONE && p != place)
> +				return PVRSGX_USSE_PLACE_NONE;
> +			place = p;
> +		}
> +	}
> +	return place;
> +}

[Severity: Medium]
Does code_place_mapped() ignore an invalid mapping when it comes before a
valid one in obj->gpuva.list?

PVRSGX_USSE_PLACE_NONE is used both as the initial value of place and as
the failure return of code_place() when va->gem.offset > va->va.addr -
base.  If the first mapping visited returns PVRSGX_USSE_PLACE_NONE, place
stays PVRSGX_USSE_PLACE_NONE, and the next iteration overwrites it with
the second mapping's p, allowing pvrsgx_ioctl_upload_code() to accept
phas.i instructions on an object mapped at conflicting locations.

[ ... ]

> +	/* VM_BIND links a mapping under the same lock. */
> +	err = dma_resv_lock_interruptible(obj->resv, NULL);
> +	if (err)
> +		goto out_buf;
> +	/* An unmapped mapping counts until its jobs and the BIF invalidate are done. */
> +	if ((!list_empty(&obj->gpuva.list) || atomic_read(&bo->code_unmapping)) &&
> +	    args->offset < bo->code_end) {
> +		err = -EBUSY;
> +		goto out_unlock;
> +	}

[Severity: Low]
Can pvrsgx_ioctl_upload_code() spuriously return -EBUSY when racing with
a failed VM_BIND?

In pvrsgx_vm.c, bind_prepare() calls drm_gpuvm_bo_obtain_locked(), which
adds an empty vm_bo to obj->gpuva.list under obj->resv.  If bind_prepare()
later fails, pvrsgx_ioctl_vm_bind() drops obj->resv via drm_exec_fini()
before re-locking it at out: to call drm_gpuvm_bo_put():

CPU0                                   CPU1
pvrsgx_ioctl_vm_bind()
  bind_prepare()
    drm_gpuvm_bo_obtain_locked()
      /* adds empty vm_bo */
  drm_exec_fini(&exec)
                                       pvrsgx_ioctl_upload_code()
                                         dma_resv_lock_interruptible()
                                         !list_empty(&obj->gpuva.list)
                                           <-- returns -EBUSY
  dma_resv_lock(ops[i].obj->resv)
  drm_gpuvm_bo_put(ops[i].vm_bo)

Checking whether any vm_bo on obj->gpuva.list has a linked drm_gpuva
avoids failing on an unmapped BO.

> +	place = code_place_mapped(obj);

[Severity: Medium]
Can pvrsgx_ioctl_upload_code() validate phas.i targets against a single
mapping while an earlier unlinked mapping is still in flight on the GPU?

When a mapping is unmapped, va_unlink_locked() increments
bo->code_unmapping and removes the va from obj->gpuva.list before
in-flight jobs and the BIF TLB invalidation finish:

CPU0                                   CPU1
pvrsgx_ioctl_vm_bind()
  /* BO mapped at P1 and P2;
     job using P1 is running */
  bind_step_unmap(P1)
    va_unlink_locked()
      atomic_inc(&bo->code_unmapping)
      drm_gpuva_unlink(va)
                                       pvrsgx_ioctl_upload_code()
                                         /* args->offset >= bo->code_end */
                                         place = code_place_mapped(obj)
                                           /* sees only P2, returns P2 */
                                         pvrsgx_usse_scan(..., P2, ...)
                                           <-- accepts phas.i for P2 while
                                               job still fetches from P1

Because atomic_read(&bo->code_unmapping) is only checked when
args->offset < bo->code_end and code_place_mapped() only walks
obj->gpuva.list, uploading phas.i at args->offset >= bo->code_end allows
the running job at P1 to jump outside the object's bounds at P1.

> +	bad = pvrsgx_usse_scan(sdev->family->isa, buf, args->size / 8, args->offset / 8,
> +			       obj->size / 8, place, &abs, &why);
> +	if (bad >= 0) {
> +		args->bad_offset = args->offset + bad * 8;
> +		drm_dbg_driver(drm, "UPLOAD_CODE: word at 0x%llx refused: %s\n",
> +			       args->bad_offset, pvrsgx_usse_refusal_name(why));
> +		err = -EINVAL;
> +		goto out_unlock;
> +	}
> +	err = drm_gem_shmem_vmap_locked(&bo->base, &map);
> +	if (err)
> +		goto out_unlock;
> +	iosys_map_memcpy_to(&map, args->offset, buf, args->size);
> +	drm_gem_shmem_vunmap_locked(&bo->base, &map);

[Severity: High]
Can a running GPU job fetch a torn 64-bit instruction during
iosys_map_memcpy_to() when uploading at args->offset >= bo->code_end?

When a code BO is mapped in a VM, bind_step_map() maps the entire range
up to obj->size into the GPU MMU, and a running task can reach offsets
at or above bo->code_end via its PDS DOUTU entry, a branch validated
against obj_insts, or fallthrough across zero words (fmad r0, r0, r0, r0).
Because uploads at args->offset >= bo->code_end are allowed while the BO
is mapped, iosys_map_memcpy_to() can race with GPU instruction fetches:

CPU0                                   GPU (USSE)
pvrsgx_ioctl_upload_code()
  /* args->offset >= bo->code_end */
  iosys_map_memcpy_to()
    /* writes upper 32 bits of
       setfc #0, #1 (0xfd100000)
       before lower 32 bits (0x100) */
                                       fetches 0xfd10000000000000
                                         <-- executes setfc #0, #0

Executing setfc #0, #0 disables C10 format control, allowing subsequent
instructions to access internal registers i64..i67 beyond USSE_I_LAST.

> +	bo->code_end = max(bo->code_end, end);

[Severity: High]
Does pvrsgx_ioctl_upload_code() miss CPU and GPU cache maintenance for
uploaded instructions?

First, when appending at args->offset >= bo->code_end to an already-mapped
code BO, the new instructions can share a GPU cache line with an earlier
shader in [0, bo->code_end) whose trailing zeroes were already cached by a
previous job, but pvrsgx_ioctl_upload_code() never sets
PVRSGX_CC_INVAL_DATA in sdev->fw.cache_control.  Second, on non-x86
platforms, uploading before VM_BIND allocates zeroed shmem pages in
drm_gem_shmem_vmap_locked() with dirty zero lines in the CPU D-cache,
writes through a write-combined vmap directly to RAM, and unpins the pages
in drm_gem_shmem_vunmap_locked(); when VM_BIND later calls
drm_gem_shmem_get_pages_sgt() -> dma_map_sgtable(), cleaning the CPU
D-cache overwrites the WC-written instructions in RAM with zeroes.

> +	if (abs) {
> +		bo->code_abs = true;
> +		bo->code_place = place;
> +	}

[Severity: Medium]
Can an unmapped, idle code BO be reused once bo->code_end and
bo->code_abs have been set?

bo->code_end only grows via max(bo->code_end, end), and bo->code_abs is
never cleared when a BO is unmapped or overwritten.  If userspace unmaps
an idle code BO to reuse it, mapping it at a new location and uploading a
shader with phas.i at offset 0 fails with -EBUSY because args->offset <
bo->code_end, and overwriting it while unmapped with code that has no
phas.i leaves bo->code_abs set so pvrsgx_validate_vm_link() rejects
mapping it anywhere other than the stale bo->code_place.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-powervr-sgx-v1-0-c69b84bac011@telaak.dev?part=11