Re: [PATCH v2 1/4] KVM: arm64: Add assignment-specific sysreg accessor

[Marc Zyngier <maz@kernel.org>]

On Tue, 03 Jun 2025 19:01:34 +0100,
Miguel Luis <miguel.luis@oracle.com> wrote:
> 
> Hi Marc,
> 
> > On 3 Jun 2025, at 07:08, Marc Zyngier <maz@kernel.org> wrote:
> > 
> > Assigning a value to a system register doesn't do what it is
> > supposed to be doing if that register is one that has RESx bits.
> > 
> > The main problem is that we use __vcpu_sys_reg(), which can be used
> > both as a lvalue and rvalue. When used as a lvalue, the bit masking
> > occurs *before* the new value is assigned, meaning that we (1) do
> > pointless work on the old cvalue, and (2) potentially assign an
> > invalid value as we fail to apply the masks to it.
> > 
> > Fix this by providing a new __vcpu_assign_sys_reg() that does
> > what it says on the tin, and sanitises the *new* value instead of
> > the old one. This comes with a significant amount of churn.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> > arch/arm64/include/asm/kvm_host.h          | 11 ++++++
> > arch/arm64/kvm/arch_timer.c                | 16 ++++----
> > arch/arm64/kvm/hyp/exception.c             |  4 +-
> > arch/arm64/kvm/hyp/include/hyp/switch.h    |  4 +-
> > arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h |  6 +--
> > arch/arm64/kvm/hyp/nvhe/hyp-main.c         |  4 +-
> > arch/arm64/kvm/hyp/vhe/switch.c            |  4 +-
> > arch/arm64/kvm/hyp/vhe/sysreg-sr.c         | 44 +++++++++++-----------
> > arch/arm64/kvm/pmu-emul.c                  | 14 +++----
> > arch/arm64/kvm/sys_regs.c                  | 38 ++++++++++---------
> > arch/arm64/kvm/sys_regs.h                  |  4 +-
> > arch/arm64/kvm/vgic/vgic-v3-nested.c       | 10 ++---
> > 12 files changed, 86 insertions(+), 73 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index d941abc6b5eef..3b84ad91116b4 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -1107,6 +1107,17 @@ 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))
> > 
> > u64 kvm_vcpu_apply_reg_masks(const struct kvm_vcpu *, enum vcpu_sysreg, u64);
> > +
> > +#define __vcpu_assign_sys_reg(v, r, val) \
> > + do { \
> > + const struct kvm_cpu_context *ctxt = &(v)->arch.ctxt; \
> > + u64 __v = (val); \
> > + if (vcpu_has_nv((v)) && (r) >= __SANITISED_REG_START__) \
> 
> Would it make sense to make it more strict by testing < NR_SYS_REGS too?

It is important to notice that you don't pass random integers here.
You pass something that is of an enum type, for which the maximum
value is NR_SYS_REGS. So as long as you pass a literal value, you're
100% safe. If you pass a constant value that goes over the limit, you
will hit the *existing* BUILD_BUG_ON().

You could fall into a trap by iterating over a base value and going
over the limit. But your check will only catch you going over the
array, and not the state corruption you will have introduced.

Finally, this is performance critical code, and I'd rather not add
extra checks that will only be a burden at runtime. If you want to
catch these overflows, using KASAN+UBSAN makes a lot more sense.

	M.

-- 
Jazz isn't dead. It just smells funny.