Re: [syzbot] [kvm?] WARNING in __kvm_gpc_refresh

[Sean Christopherson]

On Mon, Mar 18, 2024, David Woodhouse wrote:
> On Mon, 2024-03-18 at 09:25 -0700, syzbot wrote:
> > Hello,
> > 
> > syzbot found the following issue on:
> > 
> > HEAD commit:    277100b3d5fe Merge tag 'block-6.9-20240315' of git://git.k..
> > git tree:       upstream
> > console output: https://syzkaller.appspot.com/x/log.txt?x=17c96aa5180000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=1c6662240382da2
> > dashboard link: https://syzkaller.appspot.com/bug?extid=106a4f72b0474e1d1b33
> > compiler:       gcc (Debian 12.2.0-14) 122.0, GNU ld (GNU Binutils for Debian) 2.40
> > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=14358231180000
> > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=110ed231180000
> > 
> > Downloadable assets:
> > disk image (non-bootable): https://storage.googleapis.com/syzbot-assets/7bc7510fe41f/non_bootable_disk-277100b3.raw.xz
> > vmlinux: https://storage.googleapis.com/syzbot-assets/6872e049b27c/vmlinux-277100b3.xz
> > kernel image: https://storage.googleapis.com/syzbot-assets/68ec7230df0f/bzImage-277100b3.xz
> 
> static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long uhva,
>                              unsigned long len)
> {
>         unsigned long page_offset;
>         bool unmap_old = false;
>         unsigned long old_uhva;
>         kvm_pfn_t old_pfn;
>         bool hva_change = false;
>         void *old_khva;
>         int ret;
> 
>         /* 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().