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

[Sean Christopherson <seanjc@google.com>]

On Mon, Apr 01, 2024, Phi Nguyen wrote:
> On 3/29/2024 11:55 AM, syzbot wrote:
> > Hello,
> > 
> > syzbot found the following issue on:
> > 
> > HEAD commit:    928a87efa423 Merge tag 'gfs2-v6.8-fix' of git://git.kernel..
> > git tree:       upstream
> > console output: https://syzkaller.appspot.com/x/log.txt?x=127c0546180000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=f64ec427e98bccd7
> > dashboard link: https://syzkaller.appspot.com/bug?extid=dc308fcfcd53f987de73
> > 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=110481f1180000
> > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=177049a5180000
> > 
> > Downloadable assets:
> > disk image (non-bootable): https://storage.googleapis.com/syzbot-assets/7bc7510fe41f/non_bootable_disk-928a87ef.raw.xz
> > vmlinux: https://storage.googleapis.com/syzbot-assets/7979568a5a16/vmlinux-928a87ef.xz
> > kernel image: https://storage.googleapis.com/syzbot-assets/1bc6e1d480e3/bzImage-928a87ef.xz
> > 
> > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > Reported-by: syzbot+dc308fcfcd53f987de73@syzkaller.appspotmail.com

...

> > 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 syzkaller@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
> > 
> Shadow TDP
> 
> #syz test:
> git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master

> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 992e651540e8..b4275dc22d21 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -3591,7 +3591,7 @@ static void mmu_free_root_page(struct kvm *kvm, hpa_t *root_hpa,
>  void kvm_mmu_free_roots(struct kvm *kvm, struct kvm_mmu *mmu,
>                         ulong roots_to_free)
>  {
> -       bool is_tdp_mmu = tdp_mmu_enabled && mmu->root_role.direct;
> +       bool is_tdp_mmu = tdp_mmu_enabled;

This isn't a proper fix.  It would actually make a relative benign bug (taking
mmu_lock for write instead of read) far worse (taking mmu_lock for read instead
of write).  The reproducer doesn't fail because it's not actually running nested
VMs, just doing fun things with KVM_SET_CPUID2.

The issue is that kvm_mmu_after_set_cpuid() clobbers the entire role, which
results in mmu->root_role.direct being garbage.  That results in a false negative,
but as above it's quite benign as it simply means KVM takes mmu_lock for write,
when acquiring for read would suffice.

kvm_mmu_page_role.invalid already exists, we just have never used it for the
root_role.  Unless I'm missing something, the below is the simplest fix.  I'll
post a patch tomorrow, assuming testing goes well.

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 77d1072b130d..2a6c573e0c63 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -5540,9 +5540,9 @@ void kvm_mmu_after_set_cpuid(struct kvm_vcpu *vcpu)
         * that problem is swept under the rug; KVM's CPUID API is horrific and
         * it's all but impossible to solve it without introducing a new API.
         */
-       vcpu->arch.root_mmu.root_role.word = 0;
-       vcpu->arch.guest_mmu.root_role.word = 0;
-       vcpu->arch.nested_mmu.root_role.word = 0;
+       vcpu->arch.root_mmu.root_role.invalid = 1;
+       vcpu->arch.guest_mmu.root_role.invalid = 1;
+       vcpu->arch.nested_mmu.root_role.invalid = 1;
        vcpu->arch.root_mmu.cpu_role.ext.valid = 0;
        vcpu->arch.guest_mmu.cpu_role.ext.valid = 0;
        vcpu->arch.nested_mmu.cpu_role.ext.valid = 0;