linux-kernel - Re: [PATCH 2/2] KVM: SVM: Fix an error code in sev_gmem_post

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZrrbSFVpVs0eX1ZQ@google.com>
Date: Mon, 12 Aug 2024 21:04:24 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: error27@...il.com, Paolo Bonzini <pbonzini@...hat.com>, 
	Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, 
	Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org, 
	"H. Peter Anvin" <hpa@...or.com>, Brijesh Singh <brijesh.singh@....com>, 
	Michael Roth <michael.roth@....com>, Ashish Kalra <ashish.kalra@....com>, kvm@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] KVM: SVM: Fix an error code in sev_gmem_post_populate()

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@...aro.org>
> ---
>  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
>