[PATCH v1 2/4] KVM: arm64: Add compile-time type check for register in __vcpu_assign_sys_reg()

[Fuad Tabba]

Hi Marc,

On Tue, 28 Oct 2025 at 10:51, Marc Zyngier <maz at kernel.org> wrote:
>
> On Mon, 27 Oct 2025 11:39:41 +0000,
> Fuad Tabba <tabba at google.com> wrote:
> >
> > The 'r' (register) and 'val' (value) parameters in
> > __vcpu_assign_sys_reg() can be easily transposed. The register ID is an
> > enum, whereas the value is a u64. This has led to bugs in the past, and
> > the risk was increased by the historically inconsistent parameter
> > ordering of the vcpu_write_sys_reg() function.
> >
> > To prevent this class of bugs, add a compile-time type compatibility check
> > to prevent the 'r' parameter from having a 'u64' type.
> >
> > This directly catches the erroneous transposition (passing the 'u64'
> > value as 'r') while remaining flexible, as it does not force 'r' to
> > be a specific enum type.
>
> I don't think that's enough. It is too easy to pass a u32 and
> accidentally bypass this check. I really think we should redefine the
> register identifiers to be a fixed value structure, something like
> this:
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 7501a2ee4dd44..18f65dbff2379 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -624,6 +624,12 @@ enum vcpu_sysreg {
>         NR_SYS_REGS     /* Nothing after this line! */
>  };
>
> +struct vcpu_sys_reg {
> +       enum vcpu_sysreg reg;
> +};
> +
> +static const struct vcpu_sys_reg vcpu_sys_reg_SCTLR_EL1 = { SCTLR_EL1 };
> +
>  struct kvm_sysreg_masks {
>         struct {
>                 u64     res0;
>
> and then enforce that whatever is interpreted as a system register is
> actually one of these structures (with some creative cpp wrapping to
> make it less awkward).
>
> In a way, this is similar to what we are doing for page table
> manipulation, where pte_t is a structure wrapping a u64.
>
> If we move to such construct, it becomes impossible to swap value and
> register, and we can stop worrying about that.
>
> Thoughts?

I looked into this some more, and the issue is that the number of
callers who refer to members of `enum vcpu_sysreg` is really big, as
well as the functions that access its value (in if or switch
statements). It can be done, but the amount of code churn would be
huge, that in practice I don't think would be practical.

Another check I thought of, which is more robust and would catch all
cases we have encountered in practice is something along the lines of:

BUILD_BUG_ON(!__builtin_types_compatible_p(typeof(reg), enum vcpu_sysreg) &&
    !__builtin_types_compatible_p(typeof(reg), int));

This ensures that you cannot pass anything other than an enum value,
or an int, as a sysreg. U32 and U64 would trigger a build error. I
tested this, no callers needed to be changed and it did catch the bug
I am trying to avoid. It's not as robust as wrapping in a struct, but
it doesn't result in any churn.

What do you think?
/fuad


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