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

[Miguel Luis]

Hi Marc,

> On 4 Jun 2025, at 06:52, Marc Zyngier <maz at kernel.org> wrote:
> 
> On Tue, 03 Jun 2025 19:01:34 +0100,
> Miguel Luis <miguel.luis at oracle.com> wrote:
>> 
>> Hi Marc,
>> 
>>> On 3 Jun 2025, at 07:08, Marc Zyngier <maz at 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 at 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().
> 

That’s correct.

> 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.
> 

That’s also correct.

> Finally, this is performance critical code, and I'd rather not add
> extra checks that will only be a burden at runtime.

That's understandable and so far acceptable as we're trusting reg, rd->reg and r->reg
providers' code paths.

> If you want to
> catch these overflows, using KASAN+UBSAN makes a lot more sense.
> 

Thanks for your suggestion.

Thanks
Miguel

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