[syzbot] [kvm?] WARNING in __kvm_gpc_refresh

[syzbot]

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) 12.2.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

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+106a4f...@syzkaller.appspotmail.com

------------[ cut here ]------------
WARNING: CPU: 1 PID: 5192 at arch/x86/kvm/../../../virt/kvm/pfncache.c:247 __kvm_gpc_refresh+0x15e2/0x2200 arch/x86/kvm/../../../virt/kvm/pfncache.c:247
Modules linked in:
CPU: 1 PID: 5192 Comm: syz-executor422 Not tainted 6.8.0-syzkaller-11063-g277100b3d5fe #0
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
RIP: 0010:__kvm_gpc_refresh+0x15e2/0x2200 arch/x86/kvm/../../../virt/kvm/pfncache.c:247
Code: 48 c7 c2 a0 5e 02 8b be 5d 03 00 00 48 c7 c7 60 5e 02 8b c6 05 bd 89 7c 0e 01 e8 a9 23 5e 00 e9 31 fb ff ff e8 5f 85 80 00 90 <0f> 0b 90 e9 69 f7 ff ff e8 51 85 80 00 48 8b 54 24 40 48 b8 00 00
RSP: 0018:ffffc9000317f940 EFLAGS: 00010293
RAX: 0000000000000000 RBX: ffffffffffffffff RCX: ffffffff810c86ad
RDX: ffff888022360000 RSI: ffffffff810c9c31 RDI: 0000000000000000
RBP: ffff88802f2c0948 R08: 0000000000000000 R09: 0000000000000001
R10: 0000000000000001 R11: 0000000000000002 R12: ffff888000000000
R13: ffff887fffffff01 R14: 0000000000000020 R15: 0000000000000001
FS: 000055555b2d9380(0000) GS:ffff88806b300000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 000000002fa8a000 CR4: 0000000000352ef0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
kvm_gpc_refresh+0x7d/0xe0 arch/x86/kvm/../../../virt/kvm/pfncache.c:364
kvm_setup_guest_pvclock+0x5b/0x6f0 arch/x86/kvm/x86.c:3174
kvm_guest_time_update+0x935/0xeb0 arch/x86/kvm/x86.c:3313
vcpu_enter_guest arch/x86/kvm/x86.c:10769 [inline]
vcpu_run+0x1993/0x4e60 arch/x86/kvm/x86.c:11211
kvm_arch_vcpu_ioctl_run+0x42e/0x1680 arch/x86/kvm/x86.c:11437
kvm_vcpu_ioctl+0x5a1/0x1060 arch/x86/kvm/../../../virt/kvm/kvm_main.c:4464
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:904 [inline]
__se_sys_ioctl fs/ioctl.c:890 [inline]
__x64_sys_ioctl+0x193/0x220 fs/ioctl.c:890
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0xd2/0x260 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x6d/0x75
RIP: 0033:0x7fb4618fd069
Code: 48 83 c4 28 c3 e8 d7 19 00 00 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffd71e140e8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00007fb4618fd069 RCX: 00007fb4618fd069
RDX: 0000000000000000 RSI: 000000000000ae80 RDI: 0000000000000006
RBP: 00007fb46194a07e R08: 00007ffd71e14218 R09: 00007ffd71e14218
R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffd71e14150
R13: 00007ffd71e14130 R14: 00007ffd71e14120 R15: 00007fb46194a012
</TASK>


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

[Sean Christopherson]

On Mon, Mar 18, 2024, 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) 12.2.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
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+106a4f...@syzkaller.appspotmail.com
>
> ------------[ cut here ]------------
> WARNING: CPU: 1 PID: 5192 at arch/x86/kvm/../../../virt/kvm/pfncache.c:247 __kvm_gpc_refresh+0x15e2/0x2200 arch/x86/kvm/../../../virt/kvm/pfncache.c:247

The WARN is due to a new sanity check that exposed an old wart. I'll get a patch
posted later today.

[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) 12.2.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().

[David Woodhouse]

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) 12.2.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?

Paul, we should probably add ourselves to MAINTAINERS for pfncache.c

ffffffff810c8650 <__kvm_gpc_refresh>:
ffffffff810c8650: 41 57 push %r15
ffffffff810c8652: 41 56 push %r14
ffffffff810c8654: 49 89 ce mov %rcx,%r14
ffffffff810c8657: 41 55 push %r13
ffffffff810c8659: 49 bd ff ff ff ff 7f movabs $0xffff887fffffffff,%r13
ffffffff810c8660: 88 ff ff
ffffffff810c8663: 41 54 push %r12
ffffffff810c8665: 49 89 d4 mov %rdx,%r12
ffffffff810c8668: 55 push %rbp
ffffffff810c8669: 48 89 fd mov %rdi,%rbp
ffffffff810c866c: 53 push %rbx
ffffffff810c866d: 48 89 f3 mov %rsi,%rbx
ffffffff810c8670: 48 83 ec 68 sub $0x68,%rsp
ffffffff810c8674: e8 17 9b 80 00 call ffffffff818d2190 <__sanitizer_cov_trace_pc>
ffffffff810c8679: 48 89 de mov %rbx,%rsi
ffffffff810c867c: 48 c7 c7 ff ff ff ff mov $0xffffffffffffffff,%rdi
ffffffff810c8683: e8 18 96 80 00 call ffffffff818d1ca0 <__sanitizer_cov_trace_const_cmp8>
ffffffff810c8688: 48 83 fb ff cmp $0xffffffffffffffff,%rbx
ffffffff810c868c: 4c 89 ef mov %r13,%rdi
ffffffff810c868f: 4c 89 e6 mov %r12,%rsi
ffffffff810c8692: 41 0f 94 c7 sete %r15b
ffffffff810c8696: e8 05 96 80 00 call ffffffff818d1ca0 <__sanitizer_cov_trace_const_cmp8>
ffffffff810c869b: 4d 39 e5 cmp %r12,%r13
ffffffff810c869e: 44 89 ff mov %r15d,%edi
ffffffff810c86a1: 41 0f 92 c5 setb %r13b
ffffffff810c86a5: 44 89 ee mov %r13d,%esi
ffffffff810c86a8: e8 a3 94 80 00 call ffffffff818d1b50 <__sanitizer_cov_trace_cmp1>
ffffffff810c86ad: 45 38 ef cmp %r13b,%r15b
ffffffff810c86b0: 0f 84 76 15 00 00 je ffffffff810c9c2c <__kvm_gpc_refresh+0x15dc>

[David Woodhouse]

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
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)))

[Sean Christopherson]

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.

> 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().

[David Woodhouse]

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.

[Paul Durrant]

Sorry, missed this. Yes, given the changes we've made, we ought to step up.

Paul