Re: [PATCH] KVM: Avoid illegal stage2 mapping on invalid memory slot

Gavin Shan
2023-06-14T00:02:17Z

Hi Sean,

On 6/14/23 12:58 AM, Sean Christopherson wrote:
> On Tue, Jun 13, 2023, Gavin Shan wrote:
>> On 6/13/23 12:41 AM, Sean Christopherson wrote:
>>>> Fixes: 3039bcc74498 ("KVM: Move x86's MMU notifier memslot walkers to generic code")
>>>
>>> This Fixes isn't correct. That change only affected x86, which doesn't have this
>>> bug. And looking at commit cd4c71835228 ("KVM: arm64: Convert to the gfn-based MMU
>>> notifier callbacks"), arm64 did NOT skip invalid slots
>
> ...
>
>>> Unless I'm missing something, this goes all the way back to initial arm64 support
>>> added by commit d5d8184d35c9 ("KVM: ARM: Memory virtualization setup").
>>>
>>
>> The fixes tag was sorted out based on 'git-bisect', not static code analysis. I
>> agree it should be d5d8184d35c9 ("KVM: ARM: Memory virtualization setup") from
>> the code. From the 'git-bisect', we found the issue disappears when the head is
>> commit 3039bcc74498 ("KVM: Move x86's MMU notifier memslot walkers to generic code").
>> And yes, the fixes tag should be cd4c71835228 ("KVM: arm64: Convert to the gfn-based
>> MMU notifier callbacks").
>
> No, just because bisect points at a commit doesn't mean that's the commit that
> introduced a bug. It's not uncommon for a completely valid change to expose a
> pre-existing bug, which is what I suspect happened here, e.g. after switching to
> the generic framework, clean_dcache_guest_page() is called *after* retrieving the
> memslot, versus clean_dcache_guest_page() being called before walking memslots.
>
> Blaming the correct commit matters, both so that it's clear to future readers what
> went wrong, and also so that people maintaining older kernels at least are aware
> that there kernel may be affected. E.g. a fix in generic KVM obviously won't
> backport to 5.10, but someone who cares deeply about a 5.10-based kernel on arm64
> could manually port the fix to KVM arm64 code.
>

Yes, the change of the call site on clean_dcache_guest_page() enlarges the race
condition window between memory slot removal and change_pte() theoretically.
With this, we were able to reproduce the issue in a practical test. In next
revision, lets put a note about this in the changelog, like below.

We tried a git-bisect and the first problematic commit is cd4c71835228 ("
KVM: arm64: Convert to the gfn-based MMU notifier callbacks"). With this,
clean_dcache_guest_page() is called after the memory slots are iterated
in kvm_mmu_notifier_change_pte(). Without this commit, clean_dcache_guest_page()
is called before the iteration on the memory slots. This change literally
enlarges the racy window between kvm_mmu_notifier_change_pte() and memory
slot removal so that we're able to reproduce the issue in a practical test
case. However, the issue exists since d5d8184d35c9 ("KVM: ARM: Memory
virtualization setup").

Cc: stable@vger.kernel.org # v3.9+
Fixes: d5d8184d35c9 ("KVM: ARM: Memory virtualization setup")

>> I'm declined to fix the issue only for ARM64,
>
> I never suggested that an ARM-only fix be made, in fact I explicitly suggested
> the exact opposite.
>

Well, the original patch ignores the invalid memory slot for all MMU notifiers.
You suggested to have this ignorance only for change_pte(). The scope is decreased
and I was following this direction to narrow this issue/fix to change_pte() on
ARM64 only. Sorry that I misunderstood your suggestion.

>> more details are given below. If we're going to limit the issue to ARM64 and
>> fix it for ARM64 only, the fixes tag should be the one as you pointed. Lets
>> correct it in next revision with:
>
> For a generic fix, just blame multiple commits, e.g. the guilty arm64, RISC-V,
> and MIPS commits, which at a glance are the affected architectures. At one point
> in time, x86 was also likely affected, but that's probably not worth more than a
> brief mention in the changelog as I don't see any value in tracking down a very
> theoretical window of time where x86 was affected.
>

Right. Please refer to above reply. I plan to add a section in the changelog
in next revision.

>> Cc: stable@vger.kernel.org # v3.9+
>> Fixes: d5d8184d35c9 ("KVM: ARM: Memory virtualization setup")
>>
>>>> Reported-by: Shuai Hu <hshuai@redhat.com>
>>>> Reported-by: Zhenyu Zhang <zhenyzha@redhat.com>
>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>> ---
>>>> virt/kvm/kvm_main.c | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>>>> index 479802a892d4..7f81a3a209b6 100644
>>>> --- a/virt/kvm/kvm_main.c
>>>> +++ b/virt/kvm/kvm_main.c
>>>> @@ -598,6 +598,9 @@ static __always_inline int __kvm_handle_hva_range(struct kvm *kvm,
>>>> unsigned long hva_start, hva_end;
>>>> slot = container_of(node, struct kvm_memory_slot, hva_node[slots->node_idx]);
>>>> + if (slot->flags & KVM_MEMSLOT_INVALID)
>>>> + continue;
>>>
>>> Skipping the memslot will lead to use-after-free. E.g. if an invalidate_range_start()
>>> comes along between installing the invalid slot and zapping SPTEs, KVM will
>>> return from kvm_mmu_notifier_invalidate_range_start() without having dropped all
>>> references to the range.
>>>
>>> I.e.
>>>
>>> kvm_copy_memslot(invalid_slot, old);
>>> invalid_slot->flags |= KVM_MEMSLOT_INVALID;
>>> kvm_replace_memslot(kvm, old, invalid_slot);
>>>
>>> /*
>>> * Activate the slot that is now marked INVALID, but don't propagate
>>> * the slot to the now inactive slots. The slot is either going to be
>>> * deleted or recreated as a new slot.
>>> */
>>> kvm_swap_active_memslots(kvm, old->as_id);
>>>
>>>
>>> ==> invalidate_range_start()
>>>
>>> /*
>>> * From this point no new shadow pages pointing to a deleted, or moved,
>>> * memslot will be created. Validation of sp->gfn happens in:
>>> * - gfn_to_hva (kvm_read_guest, gfn_to_pfn)
>>> * - kvm_is_visible_gfn (mmu_check_root)
>>> */
>>> kvm_arch_flush_shadow_memslot(kvm, old);
>>>
>>> And even for change_pte(), skipping the memslot is wrong, as KVM would then fail
>>> unmap the prior SPTE. Of course, that can't happen in the current code base
>>> because change_pte() is wrapped with invalidate_range_{start,end}(), but that's
>>> more of a bug than a design choice (see c13fda237f08 "KVM: Assert that notifier
>>> count is elevated in .change_pte()" for details). That's also why this doesn't
>>> show up on x86; x86 installs a SPTE for the change_pte() notifier iff an existing
>>> SPTE is present, which is never true due to the invalidation.
>>>
>>
>> Right, those architectural dependencies are really something I worried about.
>
> The zap case isn't a architecture specific, that's true for all KVM architectures.
>

Yes.

>> It's safe to skip the invalid memory slots for ARM64,
>
> No, it's not. The problematic window where an invalidation can be incorrectly
> skipped is very tiny, and would have zero chance of being seen without something
> generating invalidations, e.g. page migration. But that doesn't mean there's no
> bug.
>

Yeah, Agree.

>> but it may be unsafe to do so for other architectures. You've listed the
>> potential risks to do so for x86. It might be risky with PowerPC's reverse
>> mapping stuff either. I didn't look into the code for the left architectures.
>> In order to escape from the architectural conflicts, I would move the check
>> and skip the invalid memory slot in arch/arm64/kvm/mmu.c::kvm_set_spte_gfn(),
>> something like below. In this way, the guest hang issue in ARM64 guest is
>> fixed. We may have similar issue on other architectures, but we can figure
>> out the fix when we have to. Sean, please let me know if you're happy with
>> this?
>>
>> arch/arm64/kvm/mmu.c::kvm_set_spte_gfn()
>> ----------------------------------------
>>
>> bool kvm_set_spte_gfn(struct kvm *kvm, struct kvm_gfn_range *range)
>> {
>> kvm_pfn_t pfn = pte_pfn(range->pte);
>>
>> if (!kvm->arch.mmu.pgt)
>> return false;
>>
>> /*
>> * The memory slot can become invalid temporarily or permanently
>> * when it's being moved or deleted. Avoid the stage2 mapping so
>> * that all the read and write requests to the region of the memory
>> * slot can be forwarded to VMM and emulated there.
>> */
>> if (range->slot->flags & KVM_MEMSLOT_INVALID)
>> return false;
>
> Please no. (a) it papers over common KVM's reliance on the SPTE being zapped by
> invalidate_range_start(), and (b) it leaves the same bug in RISC-V (which copy+pasted
> much of arm64's MMU code) and in MIPS (which also installs SPTEs in its callback).
>

Ok. Now I understand your suggestion to fix the issue for architectures. I will post
v3 with your suggested changes.

>> WARN_ON(range->end - range->start != 1);
>>
>> :
>> }
>>
>>> I'd honestly love to just delete the change_pte() callback, but my opinion is more
>>> than a bit biased since we don't use KSM. Assuming we keep change_pte(), the best
>>> option is probably to provide a wrapper around kvm_set_spte_gfn() to skip the
>>> memslot, but with a sanity check and comment explaining the dependency on there
>>> being no SPTEs due to the invalidation. E.g.
>>>
>>
>> It's good idea, but I'm afraid other architectures like PowerPC will still be
>> affected.
>
> I don't follow. PPC unmaps in response to a PTE change, but that's effectively
> dead code due to change_pte() being wrapped with invalidate_range_{start,end}().
>

You're right. I forgot the fact that change_pte() is surrounded by invalidate_{start,end}().

>> So I would like to limit this issue to ARM64 and fix it in its
>> kvm_set_spte_gfn() variant, as above. One question about "we don't use KSM":
>> could you please share more information about this? I'm blindly guessing
>> you're saying KSM isn't used in google cloud?
>
> Yeah, we == Google/GCE. Sorry, should have clarified that.
>

Ok, thanks for your clarification.

Thanks,
Gavin