linux-kernel - Re: UBSAN: shift-out-of-bounds in kvm_vcpu_after_set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <X/8jzqzDV35mAnIF@google.com>
Date:   Wed, 13 Jan 2021 08:46:06 -0800
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Jim Mattson <jmattson@...gle.com>,
        syzbot <syzbot+e87846c48bf72bc85311@...kaller.appspotmail.com>,
        Borislav Petkov <bp@...en8.de>,
        "H . Peter Anvin" <hpa@...or.com>, Joerg Roedel <joro@...tes.org>,
        kvm list <kvm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...hat.com>,
        syzkaller-bugs <syzkaller-bugs@...glegroups.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        the arch/x86 maintainers <x86@...nel.org>
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;