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

I thought about this some more, after our conversation yesterday, and
I still don't see how I can do it without having to change most
callsites _and_ getting strong type checking. This is of course
assuming I understood you correctly :D

For example, the most common parameter for __vcpu_assign_sys_reg() is
an `enum vcpu_sysreg`, e.g., __vcpu_assign_sys_reg(vcpu, PAR_EL1,
val);

If we wrap it as follows (after renaming __vcpu_assign_sys_reg to
___vcpu_assign_sys_reg), it still doesn't add any type checking:

define __vcpu_assign_sys_reg(vcpu, reg, val) \
do { \
   const struct vcpu_sys_reg vcpu_sys_reg ## reg = { reg }; \
    ___vcpu_assign_sys_reg(vcpu, vcpu_sys_reg, val); \
} while (0)

I can't think of a way to do it other than making sure that SCTLR_EL1
is from the beginning a `struct vcpu_sys_reg` type rather than an
`enum vcpu_sysreg` type, which means either doing a __pte(x)-like
thing at all call sites .

An alternative would be to construct a list of `static const struct
vcpu_sys_reg` for all enums, which is what you might have been
referring to above. If that's the case, what is the best way to do it?
XMacros, that would be tricky especially with the VNCR() within the
enum. Or pregenerate with a script, which seems to be too much effort
for this.

Like I mentioned, even though it's not foolproof, in practice, something like

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

If I understand the C standard correctly, enumeration constants
regardless of the size of the enum itself are always an int.
Therefore, this check is correct, even if the enum expands beyond u32
(which is very unlikely anyway), but might miss some cases if we
happen to transpose a `val` that is int with `reg` . That said, I
think that in practice it catches all the transposition cases we've
encountered.

What do you think?

Thanks,
/fuad





>
> 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?
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.