Re: UBSAN: shift-out-of-bounds in kvm_vcpu_after_set_cpuid

[Sean Christopherson <seanjc@google.com>]

On Wed, Jan 13, 2021, Paolo Bonzini wrote:
> On 12/01/21 17:53, Sean Christopherson wrote:
> > And, masking bits 7:6 is architecturally wrong.  Both the SDM and APM state that
> > bits 7:0 contain the number of PA bits.
> 
> They cannot be higher than 52,

Drat, I was going to argue that it could be >52 with a new paging mode, but both
the SDM and APM explicitly call out 52 as the max.  Spending cycles on the stuff
that really matters here... :-)

> therefore bits 7:6 are (architecturally)
> always zero.  In other words, I interpret "bit 7:0 contain the number of PA
> bits" as "you need not do an '& 63' yourself", which is basically the
> opposite of "bit 7:6 might be nonzero".  If masking made any difference, it
> would be outside the spec already.
> 
> In fact another possibility to avoid UB is to do "& 63" of both s and e in
> rsvd_bits.  This would also be masking bits 7:6 of the CPUID leaf, just done
> differently.

Hmm, 'e' is hardcoded in all call sites except kvm_mmu_reset_all_pte_masks(),
and so long as 'e <= 63' holds true, 's &= 63' is unnecessary.  What if we add
compile-time asserts on hardcoded values, and mask 'e' for the rare case where
the upper bound isn't hardcoded?  That way bogus things like rsvd_bits(63, 65)
will fail the build.

diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index 581925e476d6..261be1d2032b 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -44,8 +44,15 @@
 #define PT32_ROOT_LEVEL 2
 #define PT32E_ROOT_LEVEL 3

-static inline u64 rsvd_bits(int s, int e)
+static __always_inline u64 rsvd_bits(int s, int e)
 {
+       BUILD_BUG_ON(__builtin_constant_p(e) && __builtin_constant_p(s) && e < s);
+
+       if (__builtin_constant_p(e))
+               BUILD_BUG_ON(e > 63);
+       else
+               e &= 63;
+
        if (e < s)
                return 0;