Re: [PATCH v10 12/27] drm/xe/eudebug: Add vm open/pread/pwrite

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing runtime PM reference acquisition before accessing device memory and MMIO registers in VM read/write and fsync operations.
- [High] Missing bounds check on `timeout_ns` causes integer truncation and allows a user-controlled unbounded timeout that leads to device-wide lock starvation.
--

commit 4d072be6d2def21f4bb83f2b06e9fdfe40aeb1c5
Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>

    drm/xe/eudebug: Add vm open/pread/pwrite
    
    The debugger needs access to the client's vm to read and write, for
    example to inspect ISA/ELF and set up breakpoints.

> diff --git a/drivers/gpu/drm/xe/xe_eudebug_vm.c b/drivers/gpu/drm/xe/xe_eudebug_vm.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_eudebug_vm.c
[ ... ]
> +static ssize_t __xe_eudebug_vm_access(struct file *file,
> +				      char __user *r_buffer,
> +				      const char __user *w_buffer,
> +				      size_t count, loff_t *__pos)
> +{
[ ... ]
> +	do {
> +		const ssize_t len = min_t(ssize_t, bytes_left, alloc_len);
> +
> +		ret = __vm_read_write(vm, k_buffer,
> +				      write ? NULL : r_buffer + copied,
> +				      write ? w_buffer + copied : NULL,
> +				      (u64)(pos + copied),
> +				      len,
> +				      write);
> +		if (ret <= 0)
> +			break;
> +
> +		bytes_left -= ret;
> +		copied += ret;
> +	} while (bytes_left > 0);

[Severity: High]
Does this code need to acquire a runtime PM reference before accessing memory?

VM/VRAM access delegates down to ttm_bo_access(), which requires an active
runtime PM reference that is absent in this call chain. Because the fops
handlers execute as standard VFS file operations, they do not automatically
hold the DRM IOCTL PM runtime reference. ttm_bo_access() delegates to
xe_ttm_access_memory(), directly hitting the xe_device_assert_mem_access(xe)
assertion. If the device has entered runtime suspend, accessing memory will
crash the kernel.

[ ... ]
> +static int engine_rcu_flush(struct xe_eudebug *d,
> +			    struct xe_hw_engine *hwe,
> +			    unsigned int timeout_us)
> +{
[ ... ]
> +	/* Prevent concurrent flushes */
> +	mutex_lock(&d->xe->eudebug.lock);
> +	psmi_ctrl = xe_mmio_read32(&gt->mmio, psmi_addr);
> +	if (!(psmi_ctrl & IDLE_MSG_DISABLE))
> +		xe_mmio_write32(&gt->mmio, psmi_addr,
> +				REG_MASKED_FIELD_ENABLE(IDLE_MSG_DISABLE));
> +
> +	/* XXX: Timeout is per operation but in here we flush previous */
> +	ret = xe_mmio_wait32(&gt->mmio, RCU_ASYNC_FLUSH,
> +			     RCU_ASYNC_FLUSH_IN_PROGRESS, 0,
> +			     timeout_us, NULL, false);

[Severity: High]
Does this code block indefinitely and cause lock starvation?

There is no bounds check on timeout_ns when passed from user space, allowing
an unbounded timeout. engine_rcu_flush() implicitly truncates the 64-bit
value to a 32-bit unsigned int timeout_us. xe_mmio_wait32() will block for
this duration while holding the device-wide d->xe->eudebug.lock mutex. 

Furthermore, because the PM runtime reference is missing during fsync (as
noted below), the MMIO register read will likely return 0xFFFFFFFF, meaning
the polling loop will never succeed and will guarantee the full sleep
duration. This completely locks up the eudebug subsystem and EU page fault
processing across the entire device.

[ ... ]
> +static int xe_eudebug_vm_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> +{
[ ... ]
> +	for_each_gt(gt, d->xe, gt_id) {
> +		struct xe_hw_engine *hwe;
> +		enum xe_hw_engine_id id;
> +
> +		/* XXX: vm open per engine? */
> +		xe_eudebug_for_each_hw_engine(hwe, gt, id) {
> +			ret = engine_rcu_flush(d, hwe, vmf->timeout_us);
> +			if (ret)
> +				goto out;
> +		}
> +	}
> +out:
> +	return ret;
> +}

[Severity: High]
Does this code need to acquire a runtime PM reference before accessing MMIO
registers?

Hardware engines are iterated and MMIO registers flushed without asserting
that the device is awake via a runtime PM reference. Because
xe_eudebug_vm_fsync() executes as a standard VFS file operation, it does not
automatically hold the DRM IOCTL PM runtime reference. engine_rcu_flush()
directly accesses MMIO registers (RING_PSMI_CTL and RCU_ASYNC_FLUSH). If the
device has entered runtime suspend, accessing MMIO registers will cause PCIe
aborts or system hangs.

[ ... ]
> +long xe_eudebug_vm_open_ioctl(struct xe_eudebug *d, unsigned long arg)
> +{
[ ... ]
> +	kref_get(&d->ref);
> +	vmf->debugger = d;
> +	vmf->vm = vm;
> +	vmf->flags = param.flags;
> +	vmf->vm_handle = param.vm_handle;
> +	vmf->timeout_us = div64_u64(param.timeout_ns, 1000ull);

[Severity: High]
Can a malicious userspace client provide a huge value for timeout_ns and
cause lock starvation?

This is where the user-controlled timeout is saved without any upper bound
validation, setting up the lock starvation issue in engine_rcu_flush()
mentioned above.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903145952.848051-1-mika.kuoppala@linux.intel.com?part=12