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