linux-kernel - Re: [PATCH] RISC-V: KVM: Avoid re-acquiring memslot in kvm_riscv_gstage

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] [day] [month] [year] [list]

Message-ID: <CAK9=C2WFA+SDt4MCLj0reQnkkA2kxUmfWhT8HZxjT_DdW8W_rQ@mail.gmail.com>
Date: Sun, 15 Jun 2025 21:57:24 +0530
From: Anup Patel <apatel@...tanamicro.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Andrew Jones <ajones@...tanamicro.com>, zhouquan@...as.ac.cn, anup@...infault.org, 
	atishp@...shpatra.org, paul.walmsley@...ive.com, palmer@...belt.com, 
	linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org, 
	kvm@...r.kernel.org, kvm-riscv@...ts.infradead.org
Subject: Re: [PATCH] RISC-V: KVM: Avoid re-acquiring memslot in kvm_riscv_gstage_map()

On Sat, Jun 14, 2025 at 3:59 AM Sean Christopherson <seanjc@...gle.com> wrote:
>
> On Thu, Jun 12, 2025, Andrew Jones wrote:
> > On Wed, Jun 11, 2025 at 09:17:36AM -0700, Sean Christopherson wrote:
> > > Looks like y'all also have a bug where an -EEXIST will be returned to userspace,
> > > and will generate what's probably a spurious kvm_err() message.
> >
> > On 32-bit riscv, due to losing the upper bits of the physical address? Or
> > is there yet another thing to fix?
>
> Another bug, I think.  gstage_set_pte() returns -EEXIST if a PTE exists, and I
> _assume_ that's supposed to be benign?  But this code returns it blindly:

gstage_set_pte() returns -EEXIST only when it was expecting a non-leaf
PTE at a particular level but got a leaf PTE otherwise it returns 0 if leaf PTE
is at expected level. This allows gstage_set_pte() to work when an existing
PTE is being modified. I think the below change is not needed unless I am
totally missing something.

>
>         if (writable) {
>                 mark_page_dirty(kvm, gfn);
>                 ret = gstage_map_page(kvm, pcache, gpa, hfn << PAGE_SHIFT,
>                                       vma_pagesize, false, true);
>         } else {
>                 ret = gstage_map_page(kvm, pcache, gpa, hfn << PAGE_SHIFT,
>                                       vma_pagesize, true, true);
>         }
>
>         if (ret)
>                 kvm_err("Failed to map in G-stage\n");
>
> out_unlock:
>         kvm_release_faultin_page(kvm, page, ret && ret != -EEXIST, writable);
>         spin_unlock(&kvm->mmu_lock);
>         return ret;
>
> and gstage_page_fault() forwards negative return codes:
>
>         ret = kvm_riscv_gstage_map(vcpu, memslot, fault_addr, hva,
>                 (trap->scause == EXC_STORE_GUEST_PAGE_FAULT) ? true : false);
>         if (ret < 0)
>                 return ret;
>
> and so eventually -EEXIST will propagate to userspace.
>
> I haven't looked too closely at the RISC-V MMU, but I would be surprised if
> encountering what ends up being a spurious fault is completely impossible.
>
> > The diff looks good to me, should I test and post it for you?
>
> If you test it, I'll happily write changelogs and post patches.
>

Regards,
Anup