Re: [PATCH v2 1/2] KVM: SEV: Explicitly disallow NULL user address for SNP_LAUNCH_UPDATE

[Ackerley Tng]

Sean Christopherson <seanjc@xxxxxxxxxx> writes:

> From: Joerg Roedel <joerg.roedel@xxxxxxx>
>
> Explicitly reject a NULL userspace virtual address for the source page of
> SNP_LAUNCH_UPDATE instead of relying on the post-populate callback to do
> the check, and don't WARN on failure, as the scenario is blatantly user-
> triggerable, as reported by Sashiko.  Waiting until post-populate to check
> the address "works", but makes it unnecessarily difficult to see that KVM's
> ABI is to disallow a NULL source page for non-ZERO pages.
>
> Note, several existing VMMs pass a valid userspace address for the ZERO
> case, i.e. KVM can't *require* the userspace address to be NULL for ZERO
> pages, at least not without breaking userspace.
>
> Fixes: dee5a47cc7a4 ("KVM: SEV: Add KVM_SEV_SNP_LAUNCH_UPDATE command")
> Reported-by: Sashiko Bot <sashiko-bot@xxxxxxxxxx>
> Closes: https://lore.kernel.org/all/20260611125849.9ED631F00893@xxxxxxxxxxxxxxx
> Signed-off-by: Joerg Roedel <joerg.roedel@xxxxxxx>
> Co-developed-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
>  arch/x86/kvm/svm/sev.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 74fb15551e83..621a2eaa58f2 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2330,9 +2330,6 @@ static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
>  	int level;
>  	int ret;
>
> -	if (WARN_ON_ONCE(sev_populate_args->type != KVM_SEV_SNP_PAGE_TYPE_ZERO && !src_page))
> -		return -EINVAL;
> -
>  	ret = snp_lookup_rmpentry((u64)pfn, &assigned, &level);
>  	if (ret || assigned) {
>  		pr_debug("%s: Failed to ensure GFN 0x%llx RMP entry is initial shared state, ret: %d assigned: %d\n",
> @@ -2421,10 +2418,12 @@ static int snp_launch_update(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  	     params.type != KVM_SEV_SNP_PAGE_TYPE_CPUID))
>  		return -EINVAL;
>
> -	src = params.type == KVM_SEV_SNP_PAGE_TYPE_ZERO ? NULL : u64_to_user_ptr(params.uaddr);
> -
> -	if (!PAGE_ALIGNED(src))
> +	if (params.type == KVM_SEV_SNP_PAGE_TYPE_ZERO)
> +		src = NULL;
> +	else if (!params.uaddr || !PAGE_ALIGNED(params.uaddr))
>  		return -EINVAL;
> +	else
> +		src = u64_to_user_ptr(params.uaddr);
>

I think separating validation from src assignment logic is better, like

    if (!PAGE_ALIGNED(params.uaddr))
	return -EINVAL;

    if (!params.uaddr && params.type != KVM_SEV_SNP_PAGE_TYPE_ZERO)
        return -EINVAL;

    src = params.type == KVM_SEV_SNP_PAGE_TYPE_ZERO ? NULL :
u64_to_user_ptr(params.uaddr);

This version is also slightly stricter, since it enforces that
params.uaddr is aligned even if params.type is ZERO. (unless the
intention is to really not care about uaddr and permit an unaligned
uaddr?)

And then with conversions I'll add

    if (!gmem_in_place_conversion &&
        !params.uaddr && params.type != KVM_SEV_SNP_PAGE_TYPE_ZERO)


Just a suggestion:

Reviewed-by: Ackerley Tng <ackerleytng@xxxxxxxxxx>

>  	npages = params.len / PAGE_SIZE;
>
> --
> 2.55.0.rc0.799.gd6f94ed593-goog