Re: [RFC PATCH v1 8/8] misc/arm-cla: Add userspace interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] cla_vma_fault() unconditionally assumes FAULT_FLAG_ALLOW_RETRY is set, triggering a WARN_ON and SIGBUS during normal per-VMA lock faults.
- [High] cla_vma_open() silently ignores errors from cla_ctx_map(), leading to a context refcount imbalance and a deterministic NULL pointer dereference.
- [Medium] Concurrent ioctl queries race with CPU hotplug device setup, leaking uninitialized or partially updated accelerator descriptors to userspace.
--

--- Patch [8]: [RFC PATCH v1 8/8] misc/arm-cla: Add userspace interface ---
commit 5559b4038bdef30d19e3f3c9904b6bfa29dc03ab
Author: Ryan Roberts <ryan.roberts@arm.com>
Subject: misc/arm-cla: Add userspace interface

This commit adds a character device interface for the Arm CLA driver,
allowing userspace to enumerate hardware and map accelerator register frames.
It defines version 1 of the UAPI with a GET_PARAM ioctl for querying device
telemetry. Additionally, it implements shared read-write mmap support for
CLA register pages, utilizing a domain scheduler to resolve faults.

> diff --git a/drivers/misc/arm-cla/cla-user.c b/drivers/misc/arm-cla/cla-user.c
> new file mode 100644
> index 0000000000000..415328a653b7f
> --- /dev/null
> +++ b/drivers/misc/arm-cla/cla-user.c
[ ... ]
> @@ -0,0 +1,351 @@
[ ... ]
> +static long cla_ioctl_get_param(unsigned long arg)
> +{
[ ... ]
> +	case ARM_CLA_PARAM_DEV_ACCELS:
> +		param.value = cla_lut_pg[dev_id]->accelerators;
> +		break;
> +	case ARM_CLA_PARAM_ACCEL_IIDR:
> +		param.value = cla_lut_pg[dev_id]->accel_descs[accel_id].iidr;

[Severity: Medium]
Can concurrent ioctl queries race with CPU hotplug device setup here?

During CPU hotplug, cla_dev_setup() calls cla_dev_setup_accel() which
modifies accel_descs fields without holding dev->lock. 

Concurrently, userspace can invoke this ioctl and read these fields without
any locks or memory barriers.

Because there is no synchronization, could an out-of-order read see the
dev->accelerators bit set before the corresponding accel_descs fields are
visible, returning uninitialized or stale telemetry data to userspace?

> +		break;
> +	case ARM_CLA_PARAM_ACCEL_DEVARCH:
> +		param.value = cla_lut_pg[dev_id]->accel_descs[accel_id].devarch;
> +		break;
[ ... ]
> +static void cla_vma_open(struct vm_area_struct *vma)
> +{
> +	/*
> +	 * A vma previously created with cla_file_mmap() has been duplicated
> +	 * within the same mm (most likely due to mremap). While this could also
> +	 * be called for duplication into a new mm (via fork), we set
> +	 * VM_DONTCOPY on the original mmap, so this will never happen. So the
> +	 * contexts covered by this new vma already exist.
> +	 */
> +	struct cla_domain *domain;
> +
> +	cla_for_each_mapped_domain(vma, domain)
> +		cla_ctx_map(domain, vma->vm_mm, vma->vm_file);

[Severity: High]
Does this code need to check the return value of cla_ctx_map()?

If a VMA is duplicated (e.g., via mremap with MREMAP_MAYMOVE) and the
domain is in a broken state, cla_ctx_map() will return ERR_PTR(-EIO)
without incrementing the context refcount.

Because cla_vma_open() silently ignores this error, the refcount remains
unbalanced. When the old VMA is subsequently closed, cla_ctx_unmap() drops
the refcount to zero and frees the context.

When the new VMA is closed, cla_ctx_unmap() will fail to find the context:

drivers/misc/arm-cla/cla-ctx.c:cla_ctx_unmap() {
    ...
    ctx = cla_domain_lookup_ctx(domain, mm, file);
    WARN_ON(!ctx);

    if (!refcount_dec_and_test(&ctx->mapped)) {
    ...
}

Could this lead to an unconditional NULL pointer dereference of ctx?

> +}
> +
> +static void cla_vma_close(struct vm_area_struct *vma)
[ ... ]
> +static vm_fault_t cla_vma_fault(struct vm_fault *vmf)
> +{
> +	struct vm_area_struct *vma = vmf->vma;
> +	struct cla_domain *domain;
> +	struct cla_dev *dev;
> +	struct cla_ctx *ctx;
> +	unsigned long pg;
> +
> +	/*
> +	 * Reassignment needs mmap_lock, so we cannot wait while holding it.
> +	 * Luckily, this flag is only missing in some exotic cases which do not
> +	 * apply for this VM_IO | VM_PFNMAP vma. i.e. GUP without
> +	 * FOLL_UNLOCKABLE or FOLL_NOWAIT or fixup_user_fault(unlocked=NULL),
> +	 * used by futex fault-in path.
> +	 */
> +	if (WARN_ON(!(vmf->flags & FAULT_FLAG_ALLOW_RETRY)))
> +		return VM_FAULT_SIGBUS;

[Severity: High]
Is it safe to unconditionally assume FAULT_FLAG_ALLOW_RETRY is set here?

Under the per-VMA lock fault path (which is the default for modern kernels),
FAULT_FLAG_VMA_LOCK is set but FAULT_FLAG_ALLOW_RETRY is explicitly absent.

Consequently, any normal page fault handled under the VMA lock will hit this
branch, returning VM_FAULT_SIGBUS and breaking normal memory access.

Additionally, because this is user-controllable, could the WARN_ON allow an
unprivileged local attacker to spam the kernel log or crash the system if
panic_on_warn is enabled?

> +
> +	pg = vma->vm_pgoff + ((vmf->address - vma->vm_start) >> PAGE_SHIFT);
> +	dev = cla_lut_pg[pg];

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717104759.123203-1-ryan.roberts@arm.com?part=8