Re: [PATCH v3 18/23] KVM: arm64: Introduce kvm_pgtable_stage2_table_install()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] KVM: arm64: missing block alignment check in stage2_table_install_walker()
- [Low] KVM: arm64: kernel-doc mismatch for kvm_pgtable_stage2_table_install()
--

commit 2ec53eff489eca020eb2705b64a21720b1def67f
Author: Vincent Donnefort <vdonnefort@google.com>

KVM: arm64: Introduce kvm_pgtable_stage2_table_install()

This commit introduces kvm_pgtable_stage2_table_install() to replace a
stage-2 leaf entry with a pre-allocated table, enabling pKVM to handle block
splitting in mem_protect.c without relying on kvm_mmu_memory_cache.

> diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
> index 1c9c57cd6ae40..587874c3a5259 100644
> --- a/arch/arm64/include/asm/kvm_pgtable.h
> +++ b/arch/arm64/include/asm/kvm_pgtable.h
> @@ -835,6 +835,18 @@ int kvm_pgtable_stage2_flush(struct kvm_pgtable *pgt, u64 addr, u64 size);
>  int kvm_pgtable_stage2_split(struct kvm_pgtable *pgt, u64 addr, u64 size,
>  			     struct kvm_mmu_memory_cache *mc);
>  
> +/**
> + * kvm_pgtable_stage2_table_install() - Replace a stage-2 leaf entry with a table.
> + * @pgt:	Page-table structure initialised by kvm_pgtable_stage2_create_unlinked().
> + * @addr:	Address identifying the leaf entry to replace.
> + * @size:	Size of the block to replace.
> + * @tab:	Pointer to the pre-allocated table to install.
> + *
> + * Return: 0 on success, negative error code on failure.
> + */
> +int kvm_pgtable_stage2_table_install(struct kvm_pgtable *pgt, u64 addr, u64 size,
> +				     kvm_pte_t *table);

[Severity: Low]
In the kernel-doc comment for kvm_pgtable_stage2_table_install(), @tab does
not match the parameter name table in the prototype (and definition in
arch/arm64/kvm/hyp/pgtable.c), which triggers a scripts/kernel-doc warning.

Also, @pgt is described as initialised by
kvm_pgtable_stage2_create_unlinked(). Should @pgt be documented as
initialised by kvm_pgtable_stage2_init*(), with @table being the table that
may be created by kvm_pgtable_stage2_create_unlinked()?

> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index abd32e94f6ff0..19634e5ec5557 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c

[ ... ]

> @@ -1620,6 +1634,55 @@ int kvm_pgtable_stage2_split(struct kvm_pgtable *pgt, u64 addr, u64 size,
>  	return ret;
>  }
>  
> +struct stage2_table_install_data {
> +	struct kvm_s2_mmu	*mmu;
> +	kvm_pte_t		*table;
> +};
> +
> +static int stage2_table_install_walker(const struct kvm_pgtable_visit_ctx *ctx,
> +				       enum kvm_pgtable_walk_flags visit)
> +{
> +	struct stage2_table_install_data *data = ctx->arg;
> +	struct kvm_s2_mmu *mmu = data->mmu;
> +	kvm_pte_t *childp = data->table;
> +
> +	if (ctx->level == KVM_PGTABLE_LAST_LEVEL)
> +		return -EINVAL;
> +
> +	if (ctx->addr != ctx->start ||
> +	    ctx->end != ctx->addr + kvm_granule_size(ctx->level))
> +		return -EINVAL;

[Severity: Medium]
Does this check verify that ctx->addr is aligned to the block granule size
kvm_granule_size(ctx->level)?

In kvm_pgtable_walk(), both walk_data.start and walk_data.addr are
initialised to ALIGN_DOWN(addr, PAGE_SIZE):

	struct kvm_pgtable_walk_data walk_data = {
		.start	= ALIGN_DOWN(addr, PAGE_SIZE),
		.addr	= ALIGN_DOWN(addr, PAGE_SIZE),
		.end	= PAGE_ALIGN(walk_data.addr + size),
		.walker	= walker,
	};

In __kvm_pgtable_visit(), data->addr is only aligned down to
kvm_granule_size(level) and advanced after the leaf callback returns 0:

	if (!table) {
		data->addr = ALIGN_DOWN(data->addr, kvm_granule_size(level));
		data->addr += kvm_granule_size(level);
		goto out;
	}

Because stage2_table_install_walker() is only invoked for
KVM_PGTABLE_WALK_LEAF and always returns a non-zero value, ctx->addr is
always equal to ctx->start on the first visited leaf entry, making
ctx->addr != ctx->start always false.

If kvm_pgtable_stage2_table_install() is called with an addr that is
page-aligned but not aligned to kvm_granule_size(ctx->level) and with size
equal to kvm_granule_size(ctx->level), this check will pass, replacing the
enclosing block PTE and passing an unaligned IPA to stage2_try_break_pte().
Although the callers added later in the series (__pkvm_host_split_guest()
and host_stage2_split_gfn_meta()) check PMD_SIZE alignment beforehand,
should stage2_table_install_walker() check
!IS_ALIGNED(ctx->addr, kvm_granule_size(ctx->level))?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102204628.401871-1-vdonnefort@google.com?part=18