linux-kernel - Re: [syzbot] [kvm?] WARNING in __kvm_gpc

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: Windows password security audit tool. GUI, reports in PDF.

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

Date: Tue, 19 Mar 2024 15:57:30 +0000
From: David Woodhouse <dwmw2@...radead.org>
To: Sean Christopherson <seanjc@...gle.com>
Cc: syzbot <syzbot+106a4f72b0474e1d1b33@...kaller.appspotmail.com>, 
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org, pbonzini@...hat.com, 
	syzkaller-bugs@...glegroups.com, paul <paul@....org>
Subject: Re: [syzbot] [kvm?] WARNING in __kvm_gpc_refresh

On Tue, 2024-03-19 at 08:23 -0700, Sean Christopherson wrote:
> On Mon, Mar 18, 2024, David Woodhouse wrote:
> > On Mon, 2024-03-18 at 14:34 -0700, Sean Christopherson wrote:
> > > On Mon, Mar 18, 2024, David Woodhouse wrote:
> > > > 
> > > >         /* Either gpa or uhva must be valid, but not both */
> > > >         if (WARN_ON_ONCE(kvm_is_error_gpa(gpa) == kvm_is_error_hva(uhva)))
> > > >                 return -EINVAL;
> > > > 
> > > > Hm, that comment doesn't match the code. It says "not both", but the
> > > > code also catches the "neither" case. I think the gpa is in %rbx and
> > > > uhva is in %r12, so this is indeed the 'neither' case.
> > > > 
> > > > Is it expected that we can end up with a cache marked active, but with
> > > > the address not valid? Maybe through a race condition with deactive? or
> > > > more likely than that?
> > > 
> > > It's the darn PV system time MSR, which allows the guest to triggering activation
> > > with any GPA value.  That results in the cache being marked active without KVM
> > > ever setting the GPA (or any other fields).  The fix I'm testing is to move the
> > > offset+len check up into activate() and refresh().
> > 
> > Not sure I even want a gpc of length 1 to work at INVALID_GPA; I don't
> > think it's the offset+length check we want to be looking at?
> > 
> > If we've activated the gpc with gpa==INVALID_GPA, surely the right
> 
> This particular issue isn't due to activating with gpa==INVALID_GPA, it's due to
> marking the gpc as active without actually activating it.  The offset+length
> check is simply what causes KVM to prematurely bail from activation.

Ah, right. Yes, that makes more sense now; thanks.

> > thing to do is just let it fail (perhaps with an explicit check or just
> > letting the memslot lookup fail). After fixing that WARN_ON be
> > 
> >    if (WARN_ON_ONCE(!kvm_is_error_gpa(gpa) && !kvm_is_error_hva(uhva)))
> 
> I really don't want to relax the sanity check, as I feel strongly that KVM needs
> an invariant that an active cache is either GPA-based or HVA-based, i.e. that at
> least one of GPA or HVA is "valid".  In quotes because the GPA doesn't need to
> be fully validated, just something that doesn't trip kvm_is_error_gpa().

Agreed.

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5965 bytes)