Re: [RFC PATCH v7 17/28] KVM: arm64: Apply a RES0 mask to PMBLIMITR_EL1 writes

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi Sashiko,

On Thu, Sep 03, 2026 at 04:37:57PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Critical] get_user_spe_sysreg() and set_user_spe_sysreg() return -EBUSY before SPE initialization, violating the KVM_GET_REG_LIST contract.
> - [High] Userspace can bypass the PMBLIMITR_EL1 RES0 mask by writing to the register before the first KVM_RUN.
> - [Low] Missing READ_ONCE() when accessing pmblimitr_el1_res0_mask causes a data race.
> --
> 
> commit 1270412c2611a30b1f16a0aa9858eb290c7f9768
> Author: Alexandru Elisei <alexandru.elisei@arm.com>
> 
> KVM: arm64: Apply a RES0 mask to PMBLIMITR_EL1 writes
> 
> This commit introduces a RES0 mask for PMBLIMITR_EL1 to hide unsupported
> granule sizes from the guest. It computes this mask during the vCPU's
> first run and applies it when userspace or the guest writes to the register.
> 
> > diff --git a/arch/arm64/kvm/spe.c b/arch/arm64/kvm/spe.c
> > index 6a00a44c73edf..8c173a1f48bd4 100644
> > --- a/arch/arm64/kvm/spe.c
> > +++ b/arch/arm64/kvm/spe.c
> [ ... ]
> > @@ -60,13 +60,43 @@ bool kvm_supports_spe(void)
> [ ... ]
> >  int kvm_spe_vcpu_first_run_init(struct kvm_vcpu *vcpu)
> >  {
> > +	struct kvm *kvm = vcpu->kvm;
> > +	struct kvm_spe *kvm_spe = &kvm->arch.kvm_spe;
> >  
> > -	if (!vcpu_spe->initialized)
> > +	if (!kvm_vcpu_spe_initialized(vcpu))
> >  		return -EINVAL;
> >  
> > +	if (!kvm_spe->pmblimitr_el1_res0_mask)
> 
> [Severity: Low]
> Does this plain read of pmblimitr_el1_res0_mask need a READ_ONCE() to
> prevent compiler-induced load tearing?
> 
> The mask is updated locklessly using WRITE_ONCE() inside
> kvm_spe_compute_pmblimitr_el1_res0_mask(). Without a READ_ONCE() here,
> concurrent vCPU executions could trigger a data race.

Ok.

> 
> > +		kvm_spe_compute_pmblimitr_el1_res0_mask(kvm);
> >  
> >  	return 0;
> >  }
> > @@ -85,8 +115,12 @@ void kvm_spe_destroy_vm(struct kvm *kvm)
> >  
> >  bool kvm_spe_write_sysreg(struct kvm_vcpu *vcpu, int reg, u64 val)
> >  {
> > +	struct kvm_spe *kvm_spe = &vcpu->kvm->arch.kvm_spe;
> > +
> >  	switch (reg) {
> >  	case PMBLIMITR_EL1:
> > +		val &= ~kvm_spe->pmblimitr_el1_res0_mask;
> 
> [Severity: High]
> Can userspace bypass this RES0 sanitization?
> 
> If the VMM calls KVM_SET_ONE_REG for PMBLIMITR_EL1 before running the vCPU
> for the first time, pmblimitr_el1_res0_mask will still be 0.
> 
> Because the mask isn't computed until the first run, the bitwise NOT evaluates
> to all 1s (val &= ~0), which leaves the user-provided value unmodified and
> potentially exposes the host hardware to unsupported granule configurations.

That's correct, I assumed here that userspace will be aware of the
constraints on PMBLIMITR_EL1 due to missing architectural features and write the
correct value.

Is that placing a too high a burden on userspace? If it is, I can recompute
PMBLIMITR_EL1 when the VCPU is first run.

> 
> [Severity: Low]
> Does this mask read also need a READ_ONCE() to safely pair with the
> WRITE_ONCE() update, similar to the check in kvm_spe_vcpu_first_run_init()?

Ok.

> 
> > +		fallthrough;
> >  	case PMBSR_EL1:
> >  	case PMBPTR_EL1:
> >  		__vcpu_assign_sys_reg(vcpu, reg, val);
> 
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 89b01b6ac7830..4c4bf88c9aa53 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> [ ... ]
> > @@ -1563,6 +1563,38 @@ static unsigned int spe_visibility(const struct kvm_vcpu *vcpu,
> >  	return REG_HIDDEN;
> >  }
> >  
> > +static int set_user_spe_sysreg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
> > +			       u64 val)
> > +{
> > +	int reg = r->reg;
> > +
> > +	if (!kvm_vcpu_spe_initialized(vcpu))
> > +		return -EBUSY;
> 
> [Severity: Critical]
> Does returning -EBUSY here break the KVM_GET_REG_LIST ABI contract?
> 
> spe_visibility() currently exposes SPE registers based purely on
> vcpu_has_spe(). Because of this, KVM_GET_REG_LIST will advertise the SPE
> registers immediately after a vCPU is created with the SPE feature.
> 
> If a VMM queries the register list and then calls KVM_GET_ONE_REG or
> KVM_SET_ONE_REG before KVM_ARM_VCPU_SPE_INIT is called, it will unexpectedly
> hit this -EBUSY path and fail. The KVM API requires that any register
> returned by KVM_GET_REG_LIST must be successfully readable via
> KVM_GET_ONE_REG.

I can't help but notice an inconsistency: here you say that any register
returned by KVM_GET_REG_LIST must be successfully *readable*, yet above you're
saying that a it is a *write* that breaks the KVM_GET_REG_LIST ABI contract.

Regardless, I think you're right. I only put the check there so I can update the
interrupt level on a PMBSR_EL1 write (implemented later in the series). What I
can do is try to update the interrupt level when the VCPU is first run,
alongside applying the res0 mask to PMBLIMITR_EL1.

Thanks,
Alex

> 
> > +
> > +	if (reg == PMBLIMITR_EL1 || reg == PMBPTR_EL1 || reg == PMBSR_EL1)
> > +		kvm_spe_write_sysreg(vcpu, reg, val);
> > +	else
> > +		__vcpu_assign_sys_reg(vcpu, reg, val);
> > +
> > +	return 0;
> > +}
> > +
> > +static int get_user_spe_sysreg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
> > +			       u64 *val)
> > +{
> > +	int reg = r->reg;
> > +
> > +	if (!kvm_vcpu_spe_initialized(vcpu))
> > +		return -EBUSY;
> > +
> > +	if (reg == PMBLIMITR_EL1 || reg == PMBPTR_EL1 || reg == PMBSR_EL1)
> > +		*val = kvm_spe_read_sysreg(vcpu, reg);
> > +	else
> > +		*val = __vcpu_sys_reg(vcpu, reg);
> > +
> > +	return 0;
> > +}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260903160623.315525-1-alexandru.elisei@arm.com?part=17