Re: [PATCH v2 4/8] KVM: arm64: Correctly handle end of VA space TLBI invalidation

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Range TLBI instructions for Stage-1 TTBR1 addresses silently fail to invalidate VNCR entries because the base address is not sign-extended.
- [High] The `tlbi_va_s1_to_va` macro and `read_vncr_el2` sign-extend from bit 48, which corrupts 52-bit Virtual Addresses (LVA).
--

--- Patch [4]: [PATCH v2 4/8] KVM: arm64: Correctly handle end of VA space TLBI invalidation ---
commit 0f00ac726029153c77159941056d984a8dc753cf
Author: Marc Zyngier <[email protected]>
Subject: KVM: arm64: Correctly handle end of VA space TLBI invalidation

This commit fixes an issue with TLB invalidation by VA where address rollover
at the end of the TTBR1 VA space caused invalidations to fail. It introduces
a common helper, vncr_tlb_intersects(), which uses inclusive end addresses
to prevent this issue for both VA and IPA invalidations.

> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index 939813aebaafa..67cc5b6d80705 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
> @@ -1000,6 +1000,20 @@ static void invalidate_vncr(struct vncr_tlb *vt)
[ ... ]
> +static bool vncr_tlb_intersects(struct vncr_tlb *vt, u64 addr,
> +				u64 scope_start, u64 scope_size)
> +{
> +	u64 tlb_size, tlb_start, tlb_end, scope_end;
> +
> +	tlb_size = ttl_to_size(pgshift_level_to_ttl(vt->wi.pgshift, vt->wr.level));
> +
> +	tlb_start = addr & ~(tlb_size - 1);
> +	tlb_end = tlb_start + tlb_size - 1;
> +	scope_end = scope_start + scope_size - 1;
> +
> +	return !(tlb_end < scope_start || tlb_start > scope_end);

[Severity: High]
This is a pre-existing issue, but could this intersection check fail for
Stage-1 TTBR1 Range TLBIs due to a missing sign extension?

When a guest issues a Range TLBI for a TTBR1 virtual address,
decode_range_tlbi() zero-extends the base address:

arch/arm64/include/asm/kvm_nested.h:decode_range_tlbi() {
    ...
    base = (val & GENMASK(36, 0)) << shift;
    ...
}

In compute_s1_tlbi_range(), this is directly assigned to scope->va.

Since scope->va is passed as scope_start here and is zero-extended (a large
positive unsigned number), won't it fail to intersect with vt->gva, which
is sign-extended (a negative number for TTBR1)?

> +}
> +
>  /*
>   * VNCR TLB invalidation occurs from MMU notifiers or TLBI instructions, and
>   * either can race against a vcpu not being onlined yet (no pseudo-TLB
[ ... ]
> @@ -1060,28 +1064,19 @@ static void invalidate_vncr_va(struct kvm *kvm,
>  	lockdep_assert_held_write(&kvm->mmu_lock);
>  
>  	kvm_for_each_vncr_tlb(i, vcpu, vt, kvm) {
>  		switch (scope->type) {
>  		case TLBI_ALL:
>  			break;
>  
>  		case TLBI_VA:
> -			if (va_end <= scope->va ||
> -			    va_start >= (scope->va + scope->size))
> +			if (!vncr_tlb_intersects(vt, vt->gva, scope->va, scope->size))

[Severity: High]
This isn't a bug introduced by this patch, but does KVM incorrectly truncate
52-bit Virtual Addresses (LVA) to 48 bits when constructing these VAs?

The tlbi_va_s1_to_va() macro and read_vncr_el2() both hardcode a 48-bit sign
extension:

arch/arm64/kvm/nested.c:read_vncr_el2() {
    ...
    return (u64)sign_extend64(__vcpu_sys_reg(vcpu, VNCR_EL2), 48);
}

If a guest uses 52-bit VAs (FEAT_LVA) and places the VNCR page at a TTBR1
address where bit 48 is 0, wouldn't the upper bits (49-63) be zero-extended
instead of sign-extended?

This seems like it would corrupt the VA into a TTBR0 address, and Range TLBIs
(which preserve bits up to 53 in decode_range_tlbi) would fail to intersect
with the corrupted 48-bit vt->gva.

>  				continue;
>  			if (vt->wr.nG && vt->wr.asid != scope->asid)
>  				continue;
>  			break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4