Re: [RFC PATCH v2 13/15] misc/arm-cla: Add stage-2 MTC and VM context switch

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] kvm: use-after-free and refcount underflow in cla_ctx_create()
- [Medium] arm-cla: late validation of VHE mode breaks device domain
--

commit fd14d79171a8956e71dc07a1dfa6797aaef533a8
Author: Jean-Philippe Brucker <jpb@kernel.org>

misc/arm-cla: Add stage-2 MTC and VM context switch

Add functions to switch the guest MTC. Reset the accelerators and CLA
registers on switch. No save/restore of accelerators at the moment, we
only restore the kernel's PL0CTL.

> diff --git a/drivers/misc/arm-cla/cla-ctx.c b/drivers/misc/arm-cla/cla-ctx.c
> --- a/drivers/misc/arm-cla/cla-ctx.c
> +++ b/drivers/misc/arm-cla/cla-ctx.c
[ ... ]
> @@ -48,8 +79,12 @@ struct cla_ctx *cla_ctx_create(struct cla_domain *domain,
>  	INIT_LIST_HEAD(&ctx->queue_node);
>  	init_waitqueue_head(&ctx->waitq);
>  
> -	mmgrab(key->mm);
> -	get_file(key->file);
> +	if (cla_ctx_is_pl0(ctx)) {
> +		mmgrab(key->mm);
> +		get_file(key->file);
> +	} else {
> +		WARN_ON(!cla_ctx_get_pl1(ctx));
> +	}

[Severity: High]
If cla_ctx_get_pl1() fails and returns false (for example, if the KVM 
instance is dying and its refcount is zero), does execution continue here
with an unreferenced and potentially freed kvm pointer?

If so, this unreferenced pointer is later passed to cla_mtc_get_ctx() and
then cla_mtc_get_s2_ctx(), which calls kvm_arch_get_guest_mmu(). That
function dereferences the invalid pointer when acquiring the config_lock:

arch/arm64/kvm/mmu.c:kvm_arch_get_guest_mmu() {
        ...
        guard(mutex)(&kvm->arch.config_lock);
        ...
}

Could this lead to a use-after-free? Furthermore, if execution reaches the
err_free error path, cla_ctx_put_pl1() is called unconditionally, which
calls kvm_put_kvm() and could cause a refcount underflow. Should this path
return an error immediately if the KVM reference cannot be acquired?

> diff --git a/drivers/misc/arm-cla/cla-mtc.c b/drivers/misc/arm-cla/cla-mtc.c
> --- a/drivers/misc/arm-cla/cla-mtc.c
> +++ b/drivers/misc/arm-cla/cla-mtc.c
[ ... ]
> @@ -137,6 +205,49 @@ static int cla_mtc_uninstall_s1(struct cla_dev *dev)
>  	return cla_op_setctx(dev, cla_kernel_mtc + CLA_MTC_TTBR0, 1, &ttbr0);
>  }
>  
> +static int cla_mtc_install_s2(struct cla_dev *dev, struct cla_ctx *ctx)
> +{
> +	struct cla_mtc_s2 s2 = {};
> +	const size_t num_regs = sizeof(s2) / sizeof(u64);
> +
> +	if (WARN_ON(cla_kernel_pl != 2))
> +		return -EINVAL;

[Severity: Medium]
Because this validation occurs during the context switch path (called by
the scheduler), is it too late to gracefully reject an unsupported VM
configuration?

If this WARN_ON is hit after the PL1 context has already been created and
queued, returning an error during the context switch might cause the
hardware domain to be permanently marked as broken and leak resources.
Would it be safer to perform this capability check upfront during context
creation, for example in cla_mtc_get_s2_ctx()?

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