Re: [RFC PATCH v1 7/8] misc/arm-cla: Manage domain contexts

[sashiko-bot@kernel.org]

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 <ryan.roberts@arm.com>
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/20260717104759.123203-1-ryan.roberts@arm.com?part=7