Re: [PATCH 2/2] KVM: SVM: Fix an error code in sev_gmem_post_populate()

[Sean Christopherson <seanjc@xxxxxxxxxx>]

On Wed, Jun 12, 2024, Dan Carpenter wrote:
> The copy_from_user() function returns the number of bytes which it
> was not able to copy.  Return -EFAULT instead.

Unless I'm misreading the code and forgetting how all this works, this is
intentional.  The direct caller treats any non-zero value as a error:

		ret = post_populate(kvm, gfn, pfn, p, max_order, opaque);

		put_page(pfn_to_page(pfn));
		if (ret)
			break;
	}

	filemap_invalidate_unlock(file->f_mapping);

	fput(file);
	return ret && !i ? ret : i;

and the indirect caller specifically handles a non-zero count:

	count = kvm_gmem_populate(kvm, params.gfn_start, src, npages,
				  sev_gmem_post_populate, &sev_populate_args);
	if (count < 0) {
		argp->error = sev_populate_args.fw_error;
		pr_debug("%s: kvm_gmem_populate failed, ret %ld (fw_error %d)\n",
			 __func__, count, argp->error);
		ret = -EIO;
	} else {
		params.gfn_start += count;
		params.len -= count * PAGE_SIZE;
		if (params.type != KVM_SEV_SNP_PAGE_TYPE_ZERO)
			params.uaddr += count * PAGE_SIZE;

		ret = 0;
		if (copy_to_user(u64_to_user_ptr(argp->data), &params, sizeof(params)))
			ret = -EFAULT;
	}

and KVM's docs even call out that success doesn't mean "done".

  Upon success, this command is not guaranteed to have processed the entire
  range requested. Instead, the ``gfn_start``, ``uaddr``, and ``len`` fields of
  ``struct kvm_sev_snp_launch_update`` will be updated to correspond to the
  remaining range that has yet to be processed. The caller should continue
  calling this command until those fields indicate the entire range has been
  processed, e.g. ``len`` is 0, ``gfn_start`` is equal to the last GFN in the
  range plus 1, and ``uaddr`` is the last byte of the userspace-provided source
  buffer address plus 1. In the case where ``type`` is KVM_SEV_SNP_PAGE_TYPE_ZERO,
  ``uaddr`` will be ignored completely.

> 
> Fixes: dee5a47cc7a4 ("KVM: SEV: Add KVM_SEV_SNP_LAUNCH_UPDATE command")
> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> ---
>  arch/x86/kvm/svm/sev.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 70d8d213d401..14bb52ebd65a 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2220,9 +2220,10 @@ static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn_start, kvm_pfn_t pf
>  		if (src) {
>  			void *vaddr = kmap_local_pfn(pfn + i);
>  
> -			ret = copy_from_user(vaddr, src + i * PAGE_SIZE, PAGE_SIZE);
> -			if (ret)
> +			if (copy_from_user(vaddr, src + i * PAGE_SIZE, PAGE_SIZE)) {
> +				ret = -EFAULT;
>  				goto err;
> +			}
>  			kunmap_local(vaddr);
>  		}
>  
> -- 
> 2.43.0
>