Messages in this thread Patch in this message | | | Date | Wed, 13 Jan 2021 08:46:06 -0800 | From | Sean Christopherson <> | Subject | Re: UBSAN: shift-out-of-bounds in kvm_vcpu_after_set_cpuid |
| |
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;
| |