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 <[email protected]> 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/[email protected]?part=9
