Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Critical] Active hardware context and registers are freed when a domain is broken, allowing hardware to DMA into freed memory. - [High] Unsafe VMA iteration in `cla_domain_zap()` without holding an active mm reference, leading to use-after-free. - [High] Domain context switch silently skips devices with offline CPUs instead of failing, leading to broken context isolation. - [High] A CLA device coming online via CPU hotplug fails to restore the domain's currently assigned context. - [Medium] Missing format string argument for `%u` when creating the per-device kthread worker. --
--- Patch [7]: [RFC PATCH v1 7/8] misc/arm-cla: Manage domain contexts --- commit 62c539a6d69f0f095ec0d6a00c78d57199599362 Author: Ryan Roberts <[email protected]> Subject: misc/arm-cla: Manage domain contexts A CLA domain cannot be used by multiple address spaces concurrently. This commit introduces context tracking and scheduling for CLA domains, running hardware operations via kthread workers. > diff --git a/drivers/misc/arm-cla/cla-init.c b/drivers/misc/arm-cla/cla-init.c > index 0451d97bc95b3..5335c55568add 100644 > --- a/drivers/misc/arm-cla/cla-init.c > +++ b/drivers/misc/arm-cla/cla-init.c [ ... ] > @@ -171,6 +172,46 @@ static void cla_dev_reinit(struct cla_dev *dev) > cla_reg_write(dev, CLA_REG_LRESP, 0); > } > > +static int cla_dev_worker_init(struct cla_dev *dev, int cpu) > +{ > + struct kthread_worker *worker; > + > + worker = kthread_run_worker_on_cpu(cpu, 0, "cla-dev-worker/%u"); [Severity: Medium] Is it possible this leaks uninitialized memory to user-space? The string "cla-dev-worker/%u" expects an integer argument, but the wrapper macro kthread_run_worker_on_cpu() only takes the name format and does not forward arguments to the underlying kthread_create_worker_on_cpu() function. Will this cause vsnprintf() to consume a random stack slot or register for the thread name? > + if (IS_ERR(worker)) > + return PTR_ERR(worker); > + [ ... ] > @@ -227,6 +268,10 @@ static int cla_dev_setup(unsigned int cpu) > dev->accelerators); > } > > + ret = cla_dev_worker_init(dev, cpu); > + if (ret) > + goto err; > + > + return 0; [Severity: High] When a CPU is hotplugged back online, will this setup path leave the hardware out of sync with the domain? It initializes the worker but does not appear to check domain->assigned_ctx or queue work to restore the active context to the hardware. If the domain only has one active context, a subsequent reassignment will abort in cla_domain_reassign() because next_ctx and prev_ctx evaluate to NULL. Can this permanently bypass context restoration and cause memory faults? > err: > cla_dev_reinit(dev); > diff --git a/drivers/misc/arm-cla/cla-sched.c > b/drivers/misc/arm-cla/cla-sched.c > new file mode 100644 > index 0000000000000..18b54cc386b7c > --- /dev/null > +++ b/drivers/misc/arm-cla/cla-sched.c [ ... ] > +static int __cla_domain_switch_ctx(struct cla_domain *domain, > + struct cla_ctx *prev_ctx, > + struct cla_ctx *next_ctx) > +{ [ ... ] > + for (i = 0; i < domain->nr_devs; i++) { > + dev = domain->devs[i]; > + call = &dev->call; > + > + call->sched.prev_ctx = prev_ctx; > + call->sched.next_ctx = next_ctx; > + call->sched.ctx_id = i; > + call->ret = 0; > + > + mutex_lock(&dev->lock); > + if (dev->worker && !dev->broken) > + kthread_queue_work(dev->worker, &call->switch_ctx); > + mutex_unlock(&dev->lock); [Severity: High] If dev->worker is NULL because the CPU is offline, does this silently bypass the context switch for that device? Since call->ret is initialized to 0 and there is no else branch to return an error, the domain switch will complete and report success. When the CPU comes back online, could the device retain a stale context and break isolation? > + } [ ... ] > +static void cla_domain_zap(struct cla_domain *domain, struct cla_ctx *ctx) > +{ > + struct vm_area_struct *vma; > + struct vma_iterator vmi; > + > + /* > + * If the context is dying it has already been unmapped, no need to zap > + * it here. > + */ > + if (!ctx || cla_ctx_is_dying(ctx)) > + return; > + > + /* > + * Iterate over vmas in prev_ctx's mm, looking for vmas that map > + * prev_ctx's file. Zap any portions of those vmas that map the domain. > + */ > + mmap_read_lock(ctx->key.mm); > + vma_iter_init(&vmi, ctx->key.mm, 0); > + for_each_vma(vmi, vma) { [Severity: High] Is it safe to acquire the mmap_read_lock() and iterate the maple tree here without an active mm reference? The context only holds a passive mmgrab() reference. If a concurrent unprivileged userspace process exits, mm_users could drop to 0, and exit_mmap() might tear down the VMA maple tree. Should this code use mmget_not_zero() to prevent a use-after-free while walking the tree? > + if (vma->vm_file != ctx->key.file) > + continue; [ ... ] > +static void cla_domain_reclaim(struct kthread_work *work) > +{ > + struct cla_domain *domain; > + struct cla_ctx *ctx, *next; > + > + domain = container_of(work, struct cla_domain, reclaim.work); > + > + mutex_lock(&domain->lock); > + list_for_each_entry_safe(ctx, next, &domain->dying_ctxs, queue_node) { > + list_del_init(&ctx->queue_node); > + WARN_ON(domain->assigned_ctx == ctx); > + cla_ctx_put(ctx); [Severity: Critical] If a domain becomes broken, is it possible for the active hardware context to be freed while still programmed in the device? In cla_domain_schedule_reassignment(), the reassign work is skipped if the domain is broken, meaning the context is never uninstalled from the hardware. Then here in cla_domain_reclaim(), the dying context is unconditionally freed by calling cla_ctx_put(), which drops the PGD and software state. Will this allow the broken hardware to perform page walks into freed and reallocated memory? > + } > + mutex_unlock(&domain->lock); > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
