Received-SPF: pass (google.com: domain of linux-kernel+bounces-107703-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 19 Mar 2024 08:23:30 -0700
In-Reply-To: <33bcc5778e39780c6895ffa9f52f4b12cf83ad89.camel@infradead.org>
Precedence: bulk
Mime-Version: 1.0
References: <0000000000005fa5cc0613f1cebd@google.com> <b7561e6d6d357fcd8ec1a1257aaf2f97d971061c.camel@infradead.org>
 <ZfizYzC9-9Qo47tE@google.com> <33bcc5778e39780c6895ffa9f52f4b12cf83ad89.camel@infradead.org>
Message-ID: <Zfmt8rxlF1ag1iA_@google.com>
Subject: Re: [syzbot] [kvm?] WARNING in __kvm_gpc_refresh
From: Sean Christopherson <seanjc@google.com>
To: David Woodhouse <dwmw2@infradead.org>
Cc: syzbot <syzbot+106a4f72b0474e1d1b33@syzkaller.appspotmail.com>, 
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org, pbonzini@redhat.com, 
	syzkaller-bugs@googlegroups.com, paul <paul@xen.org>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

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:
> > >=20
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* Either gpa or uhva must=
 be valid, but not both */
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (WARN_ON_ONCE(kvm_is_er=
ror_gpa(gpa) =3D=3D kvm_is_error_hva(uhva)))
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0 return -EINVAL;
> > >=20
> > > 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.
> > >=20
> > > Is it expected that we can end up with a cache marked active, but wit=
h
> > > the address not valid? Maybe through a race condition with deactive? =
or
> > > more likely than that?
> >=20
> > It's the darn PV system time MSR, which allows the guest to triggering =
activation
> > with any GPA value.=C2=A0 That results in the cache being marked active=
 without KVM
> > ever setting the GPA (or any other fields).=C2=A0 The fix I'm testing i=
s to move the
> > offset+len check up into activate() and refresh().
>=20
> 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?
>=20
> If we've activated the gpc with gpa=3D=3DINVALID_GPA, surely the right

This particular issue isn't due to activating with gpa=3D=3DINVALID_GPA, it=
's due to
marking the gpc as active without actually activating it.  The offset+lengt=
h
check is simply what causes KVM to prematurely bail from activation.

> 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
>=20
>    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. th=
at 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().