Re: [PATCH] KVM: arm64: Correctly cap TLBI Range to the architural limit

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

New issues:
- [High] Hardcoded BIT(48) cap truncates valid 52-bit IPAs and underflows for higher addresses
- [High] Upper-half range cap `~base + 1` is completely ineffective due to missing sign-extension

Pre-existing issues:
- [High] Range TLB invalidations for upper-half VAs silently fail to invalidate the VNCR cache
- [High] Malicious guests can still trigger a host WARN_ON because the BIT(48) cap ignores smaller VM ia_bits
--

--- Patch [1]: [PATCH] KVM: arm64: Correctly cap TLBI Range to the architural limit ---
commit 09ab12a5dd613f7e322a9123897db5c50369508d
Author: Marc Zyngier <[email protected]>

KVM: arm64: Correctly cap TLBI Range to the architural limit

This commit caps the range of a TLB invalidation based on the base address
to prevent guests from triggering an overflow when the hypervisor adds the
range to the base address.

Link: https://lore.kernel.org/r/yifz3wn5gk5sr6mapi32trgk5m5kp33bquctsjmkifebnsnndt@fix6u4rthx4g

> diff --git a/arch/arm64/include/asm/kvm_nested.h b/arch/arm64/include/asm/kvm_nested.h
> index cdf3e8422ea12..b68806b3bd6e5 100644
> --- a/arch/arm64/include/asm/kvm_nested.h
> +++ b/arch/arm64/include/asm/kvm_nested.h
> @@ -298,6 +298,12 @@ static inline u64 decode_range_tlbi(u64 val, u64 *range, u16 *asid)
>  	num	= FIELD_GET(GENMASK(43, 39), val);
>  	*range	= __TLBI_RANGE_PAGES(num, scale) << shift;

[Severity: High]
This isn't a bug introduced by this patch, but earlier in
decode_range_tlbi(), base is extracted without sign-extension:

	base = (val & GENMASK(36, 0)) << shift;

For upper-half kernel virtual addresses, base will be zero-extended. When
invalidate_vncr_va() in arch/arm64/kvm/nested.c checks if this invalidation
intersects with a cached VNCR TLB (vt->gva), the cached TLB is correctly
sign-extended. The bounds check incorrectly fails due to comparing a
sign-extended gva against a zero-extended scope->va:

	if (va_end <= scope->va ||
	    va_start >= (scope->va + scope->size))
		continue;

Does this leave stale entries in the VNCR cache and break memory isolation
for nested guests when they invalidate an upper-half VA range via
TLBI RVAE1*?

> +
> +	/* Cap the range to the correct half of the address space */
> +	if (!(base & BIT(48)))
> +		*range = min(*range, (BIT(48) - base));

[Severity: High]
Does this hardcoded BIT(48) cap incorrectly truncate valid 52-bit IPAs?

For guests with a 52-bit IPA space (FEAT_LPA), a valid TLB invalidation over
the entire address space (base=0, range=BIT(52)) appears to be truncated to
BIT(48), potentially leaving stale mappings in the upper 48-52 bits.

Additionally, if the base address exceeds BIT(48) but has bit 48 cleared
(for example, base=BIT(49)), won't (BIT(48) - base) underflow to a massive
unsigned value, bypassing the cap and failing to prevent the overflow?

[Severity: High]
This is a pre-existing issue, but since this cap uses BIT(48), can a nested
guest still trigger the host WARN_ON in __unmap_stage2_range() if the VM's
configured PARange (ia_bits) is smaller than 48?

If a guest supplies a base and range that stays below BIT(48) but exceeds
the VM's actual limit (for example, BIT(40)), it seems _kvm_pgtable_walk() in
arch/arm64/kvm/hyp/pgtable.c will still return -ERANGE:

	if (data->addr > limit || data->end > limit)
		return -ERANGE;

This bypasses the new cap and can trigger the warning, which could act as a
Denial of Service if panic_on_warn is set.

> +	else
> +		*range = min(*range, ~base + 1);

[Severity: High]
Is this upper-half range cap mathematically effective without base being
sign-extended?

Since decode_range_tlbi() leaves base as a positive unsigned 64-bit integer,
~base + 1 evaluates to an extremely large positive value near 2^64. Will
min(*range, ~base + 1) always evaluate to *range, leaving the range uncapped
and still vulnerable to the overflow for upper-half virtual addresses?

> +
>  	return base;
>  }
>

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