Re: [PATCH 03/25] KVM: arm64: nv: Add sanitising to VNCR-backed sysregs

[Marc Zyngier <maz@kernel.org>]

On Tue, 23 Jan 2024 13:48:57 +0000,
Joey Gouly <joey.gouly@arm.com> wrote:
> 
> Hi,
> 
> On Mon, Jan 22, 2024 at 08:18:30PM +0000, Marc Zyngier wrote:
> > VNCR-backed "registers" are actually only memory. Which means that
> > there is zero control over what the guest can write, and that it
> > is the hypervisor's job to actually sanitise the content of the
> > backing store. Yeah, this is fun.
> > 
> > In order to preserve some form of sanity, add a repainting mechanism
> > that makes use of a per-VM set of RES0/RES1 masks, one pair per VNCR
> > register. These masks get applied on access to the backing store via
> > __vcpu_sys_reg(), ensuring that the state that is consumed by KVM is
> > correct.
> > 
> > So far, nothing populates these masks, but stay tuned.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/include/asm/kvm_host.h | 25 +++++++++++++++++++
> >  arch/arm64/kvm/arm.c              |  1 +
> >  arch/arm64/kvm/nested.c           | 41 ++++++++++++++++++++++++++++++-
> >  3 files changed, 66 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index c0cf9c5f5e8d..fe35c59214ad 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -238,6 +238,8 @@ static inline u16 kvm_mpidr_index(struct kvm_mpidr_data *data, u64 mpidr)
> >  	return index;
> >  }
> >  
> > +struct kvm_sysreg_masks;
> > +
> >  struct kvm_arch {
> >  	struct kvm_s2_mmu mmu;
> >  
> > @@ -312,6 +314,9 @@ struct kvm_arch {
> >  #define KVM_ARM_ID_REG_NUM	(IDREG_IDX(sys_reg(3, 0, 0, 7, 7)) + 1)
> >  	u64 id_regs[KVM_ARM_ID_REG_NUM];
> >  
> > +	/* Masks for VNCR-baked sysregs */
> > +	struct kvm_sysreg_masks	*sysreg_masks;
> > +
> >  	/*
> >  	 * For an untrusted host VM, 'pkvm.handle' is used to lookup
> >  	 * the associated pKVM instance in the hypervisor.
> > @@ -474,6 +479,13 @@ enum vcpu_sysreg {
> >  	NR_SYS_REGS	/* Nothing after this line! */
> >  };
> >  
> > +struct kvm_sysreg_masks {
> > +	struct {
> > +		u64	res0;
> > +		u64	res1;
> > +	} mask[NR_SYS_REGS - __VNCR_START__];
> > +};
> > +
> >  struct kvm_cpu_context {
> >  	struct user_pt_regs regs;	/* sp = sp_el0 */
> >  
> > @@ -868,7 +880,20 @@ static inline u64 *__ctxt_sys_reg(const struct kvm_cpu_context *ctxt, int r)
> >  
> >  #define ctxt_sys_reg(c,r)	(*__ctxt_sys_reg(c,r))
> >  
> > +#if defined (__KVM_NVHE_HYPERVISOR__)
> >  #define __vcpu_sys_reg(v,r)	(ctxt_sys_reg(&(v)->arch.ctxt, (r)))
> > +#else
> > +u64 kvm_vcpu_sanitise_vncr_reg(const struct kvm_vcpu *, enum vcpu_sysreg);
> > +#define __vcpu_sys_reg(v,r)						\
> > +	(*({								\
> > +		const struct kvm_cpu_context *ctxt = &(v)->arch.ctxt;	\
> > +		u64 *__r = __ctxt_sys_reg(ctxt, (r));			\
> > +		if (unlikely(cpus_have_final_cap(ARM64_HAS_NESTED_VIRT) && \
> > +			     r >= __VNCR_START__ && ctxt->vncr_array))	\
> > +			*__r = kvm_vcpu_sanitise_vncr_reg((v), (r));	\
> > +		__r;							\
> > +	}))
> > +#endif
> 
> Can you not use vcpu_has_nv() here? I see that __ctxt_sys_reg() does a similar
> check, but vcpu_has_nv() covers !__KVM_NVHE_HYPERVISOR__, ARM64_HAS_NESTED_VIRT
> and KVM_ARM_VCPU_HAS_EL2 (which I guess is what the ctxt->vncr_array check is
> doing?) I can see it's defined in kvm_nested.h, which includes kvm_host.h, so
> maybe that's an issue.
> 
> #define __vcpu_sys_reg(v,r)						\
> 	(*({								\
> 		const struct kvm_cpu_context *ctxt = &(v)->arch.ctxt;	\
> 		u64 *__r = __ctxt_sys_reg(ctxt, (r));			\
> 		if (unlikely(vcpu_has_nv(v) && r >= __VNCR_START__))	\
> 			*__r = kvm_vcpu_sanitise_vncr_reg((v), (r));	\
> 		__r;							\
> 	}))
> 
> And since vcpu_has_nv() already checks __KVM_NVHE_HYPERVISOR__, you don't need
> to define __vcpu_sys_reg() twice.

All good points. Now that we only cater for NV2, vncr_array not being
NULL is a given, although we still need it in __ctxt_sys_reg() as we
don't have the full-fat vcpu at this stage (and thus cannot check for
flags).

>
> Also maybe move that derefence into the macro, like: *__r;, instead of being
> after the first (.

Surprisingly, this doesn't work:

<quote>
./arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h:240:38: error: lvalue required as left operand of assignment
240 |   __vcpu_sys_reg(vcpu, DBGVCR32_EL2) = read_sysreg(dbgvcr32_el2);

</quote>

There are plenty more.

> I'm not sure about the ctxt->vncr_array check, so maybe that's still
> important.

In the absence of the flag, it is. And I'm actually tempted to
standardise on checking for vncr_array in vcpu_has_nv() as a
substitute for the flag. It is likely to be a bit cheaper and for the
value to be needed down the line.

I'll rework this shortly.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.