Re: [RFC PATCH v2 09/15] misc/arm-cla: Add userspace interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] core: format string mismatch in dev_set_name causes init failure on 32-bit archs
- [Medium] misc/arm-cla: missing array_index_nospec in cla_vma_fault creates Spectre v1 gadget
--

commit 91161eb49b1b380686ad6f07674f5b53c3cf7baa
Author: Ryan Roberts <ryan.roberts@arm.com>

    misc/arm-cla: Add userspace interface
    
    Expose CLA devices through a character device so userspace can enumerate
    the available hardware and map accelerator register frames.

> diff --git a/drivers/misc/arm-cla/cla-user.c b/drivers/misc/arm-cla/cla-user.c
> new file mode 100644
> index 0000000000000..41cd5e4a7d017
> --- /dev/null
> +++ b/drivers/misc/arm-cla/cla-user.c
> @@ -0,0 +1,541 @@
[ ... ]
> +static vm_fault_t cla_vma_fault(struct vm_fault *vmf)
> +{
> +	struct vm_area_struct *vma = vmf->vma;
> +	struct cla_ctx_key key = CLA_CTX_KEY_PL0(vma->vm_mm, vma->vm_file);
> +	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.
> +	 * The private futex fault-in path can reach here without allowing
> +	 * mmap_lock to be dropped. Reject faults that do not allow retry.
> +	 */
> +	if (!(vmf->flags & FAULT_FLAG_ALLOW_RETRY))
> +		return VM_FAULT_SIGBUS;
> +
> +	pg = vma->vm_pgoff + ((vmf->address - vma->vm_start) >> PAGE_SHIFT);
> +	dev = cla_lut_pg[pg];
> +	domain = dev->domain;

[Severity: Medium]
In cla_vma_fault(), pg is calculated from the user-controlled vmf->address
and used to index the global cla_lut_pg array.

Is there a risk that speculative execution could bypass the VMA bounds checks
prior to this handler, resulting in an out-of-bounds read that is immediately
dereferenced? Could this create a Spectre v1 gadget, and should we use
array_index_nospec() to clamp pg before indexing?

> +
> +	mutex_lock(&domain->lock);
> +	ctx = cla_domain_lookup_ctx(domain, &key);
[ ... ]
> +static int __init cla_accel_class_add(const struct cla_accel_desc *desc)
> +{
> +	struct cla_accel_class *aclass;
> +	unsigned int i;
> +	int ret;
[ ... ]
> +	aclass->desc = *desc;
> +	device_initialize(&aclass->dev);
> +	aclass->dev.class = cla_class;
> +	aclass->dev.devt = cla_devt + cla_nr_classes;
> +	aclass->dev.release = cla_accel_class_release;
> +
> +	ret = dev_set_name(&aclass->dev, "%03lx:%03lx",
> +			   FIELD_GET(CLA_IIDR_IMPLEMENTER, desc->iidr),
> +			   FIELD_GET(CLA_IIDR_PRODUCTID, desc->iidr));

[Severity: High]
Does this format string cause argument misalignment on 32-bit architectures?

Because desc->iidr is a u64, FIELD_GET() returns a 64-bit value. Using %03lx
expects a 32-bit unsigned long on 32-bit systems. This could cause the first
%03lx to consume the lower 32 bits, and the second %03lx to consume the upper
32 bits, completely ignoring the intended PRODUCTID argument.

If devices share the same implementer but have different product IDs, could
this result in identical names and cause device_add() to fail with -EEXIST?

> +	if (ret)
> +		goto err_put;
> +
> +	/* The node cannot be opened until cla_user_init() adds the cdev. */
> +	ret = device_add(&aclass->dev);

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