[PATCH 1/2] KVM: SVM: Fix uninitialized variable bug

[PATCH 1/2] KVM: SVM: Fix uninitialized variable bug

[Dan Carpenter]

If snp_lookup_rmpentry() fails then "assigned" is printed in the error
message but it was never initialized.  Initialize it to false.

Fixes: dee5a47cc7a4 ("KVM: SEV: Add KVM_SEV_SNP_LAUNCH_UPDATE command")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
The compiler is generally already zeroing stack variables so this doesn't cost
anything.

 arch/x86/kvm/svm/sev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 43a450fb01fd..70d8d213d401 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2199,7 +2199,7 @@ static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn_start, kvm_pfn_t pf
 
 	for (gfn = gfn_start, i = 0; gfn < gfn_start + npages; gfn++, i++) {
 		struct sev_data_snp_launch_update fw_args = {0};
-		bool assigned;
+		bool assigned = false;
 		int level;
 
 		if (!kvm_mem_is_private(kvm, gfn)) {
-- 
2.43.0

Re: [PATCH 1/2] KVM: SVM: Fix uninitialized variable bug

[Sean Christopherson]

On Wed, Jun 12, 2024, Dan Carpenter wrote:
> If snp_lookup_rmpentry() fails then "assigned" is printed in the error
> message but it was never initialized.  Initialize it to false.
> 
> Fixes: dee5a47cc7a4 ("KVM: SEV: Add KVM_SEV_SNP_LAUNCH_UPDATE command")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> The compiler is generally already zeroing stack variables so this doesn't cost
> anything.
> 
>  arch/x86/kvm/svm/sev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 43a450fb01fd..70d8d213d401 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2199,7 +2199,7 @@ static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn_start, kvm_pfn_t pf
>  
>  	for (gfn = gfn_start, i = 0; gfn < gfn_start + npages; gfn++, i++) {
>  		struct sev_data_snp_launch_update fw_args = {0};
> -		bool assigned;
> +		bool assigned = false;

I would rather delete all the printks, or if people really like the printks, at
least provide some helpers to dedup the code.  E.g. sev_gmem_prepare() has more
or less the exact same behavior, but doesn't have the same flaw.

>  		int level;
>  
>  		if (!kvm_mem_is_private(kvm, gfn)) {
> -- 
> 2.43.0
> 

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

[Dan Carpenter]

The copy_from_user() function returns the number of bytes which it
was not able to copy.  Return -EFAULT instead.

Fixes: dee5a47cc7a4 ("KVM: SEV: Add KVM_SEV_SNP_LAUNCH_UPDATE command")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.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

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

[Sean Christopherson]

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@linaro.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
> 

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

[Dan Carpenter]

On Mon, Aug 12, 2024 at 09:04:24PM -0700, Sean Christopherson wrote:
> 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;
> 

No, you're not reading this correctly.  The loop is supposed to return the
number of pages which were handled successfully.  So this is saying that if the
first iteration fails and then return a negative error code.  But with the bug
then if the first iteration fails, it returns the number of bytes which failed.
The units are wrong pages vs bytes and the failure vs success is reversed.

Also I notice now that i isn't correct unless we hit a break statement:

virt/kvm/guest_memfd.c
   647          npages = min_t(ulong, slot->npages - (start_gfn - slot->base_gfn), npages);

If there isn't enough pages, we use what's available.

   648          for (i = 0; i < npages; i += (1 << max_order)) {

If we exit because i >= npages then we return success as if we were able to
complete one final iteration through the loop.

   649                  struct folio *folio;
   650                  gfn_t gfn = start_gfn + i;
   651                  bool is_prepared = false;
   652                  kvm_pfn_t pfn;
   653  
   654                  if (signal_pending(current)) {
   655                          ret = -EINTR;
   656                          break;
   657                  }
   658  
   659                  folio = __kvm_gmem_get_pfn(file, slot, gfn, &pfn, &is_prepared, &max_order);
   660                  if (IS_ERR(folio)) {
   661                          ret = PTR_ERR(folio);
   662                          break;
   663                  }
   664  
   665                  if (is_prepared) {
   666                          folio_unlock(folio);
   667                          folio_put(folio);
   668                          ret = -EEXIST;
   669                          break;
   670                  }
   671  
   672                  folio_unlock(folio);
   673                  WARN_ON(!IS_ALIGNED(gfn, 1 << max_order) ||
   674                          (npages - i) < (1 << max_order));
   675  
   676                  ret = -EINVAL;
   677                  while (!kvm_range_has_memory_attributes(kvm, gfn, gfn + (1 << max_order),
   678                                                          KVM_MEMORY_ATTRIBUTE_PRIVATE,
   679                                                          KVM_MEMORY_ATTRIBUTE_PRIVATE)) {
   680                          if (!max_order)
   681                                  goto put_folio_and_exit;
   682                          max_order--;
   683                  }
   684  
   685                  p = src ? src + i * PAGE_SIZE : NULL;
   686                  ret = post_populate(kvm, gfn, pfn, p, max_order, opaque);
                        ^^^^^^^^^^^^^^^^^^^^
post_populate() is a pointer to sev_gmem_post_populate() which has is supposed
to return negative error codes but instead returns number of bytes which failed.

   687                  if (!ret)
   688                          kvm_gmem_mark_prepared(folio);
   689  
   690  put_folio_and_exit:
   691                  folio_put(folio);
   692                  if (ret)
   693                          break;
   694          }
   695  
   696          filemap_invalidate_unlock(file->f_mapping);
   697  
   698          fput(file);
   699          return ret && !i ? ret : i;
   700  }

regards,
dan carpenter

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

[Paolo Bonzini]

On 8/13/24 09:51, Dan Carpenter wrote:
> On Mon, Aug 12, 2024 at 09:04:24PM -0700, Sean Christopherson wrote:
>> 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;
>>
> 
> No, you're not reading this correctly.  The loop is supposed to return the
> number of pages which were handled successfully.  So this is saying that if the
> first iteration fails and then return a negative error code.  But with the bug
> then if the first iteration fails, it returns the number of bytes which failed.

Yes, you're supposed to return 0 or -errno, so that you return -errno on 
the first round.  Applying the patches, 1/2 is also a bugfix even if the 
printks may be overkill.

Thanks for the report!

Paolo

> The units are wrong pages vs bytes and the failure vs success is reversed.
> 
> Also I notice now that i isn't correct unless we hit a break statement:
> 
> virt/kvm/guest_memfd.c
>     647          npages = min_t(ulong, slot->npages - (start_gfn - slot->base_gfn), npages);
> 
> If there isn't enough pages, we use what's available.
> 
>     648          for (i = 0; i < npages; i += (1 << max_order)) {
> 
> If we exit because i >= npages then we return success as if we were able to
> complete one final iteration through the loop.
> 
>     649                  struct folio *folio;
>     650                  gfn_t gfn = start_gfn + i;
>     651                  bool is_prepared = false;
>     652                  kvm_pfn_t pfn;
>     653
>     654                  if (signal_pending(current)) {
>     655                          ret = -EINTR;
>     656                          break;
>     657                  }
>     658
>     659                  folio = __kvm_gmem_get_pfn(file, slot, gfn, &pfn, &is_prepared, &max_order);
>     660                  if (IS_ERR(folio)) {
>     661                          ret = PTR_ERR(folio);
>     662                          break;
>     663                  }
>     664
>     665                  if (is_prepared) {
>     666                          folio_unlock(folio);
>     667                          folio_put(folio);
>     668                          ret = -EEXIST;
>     669                          break;
>     670                  }
>     671
>     672                  folio_unlock(folio);
>     673                  WARN_ON(!IS_ALIGNED(gfn, 1 << max_order) ||
>     674                          (npages - i) < (1 << max_order));
>     675
>     676                  ret = -EINVAL;
>     677                  while (!kvm_range_has_memory_attributes(kvm, gfn, gfn + (1 << max_order),
>     678                                                          KVM_MEMORY_ATTRIBUTE_PRIVATE,
>     679                                                          KVM_MEMORY_ATTRIBUTE_PRIVATE)) {
>     680                          if (!max_order)
>     681                                  goto put_folio_and_exit;
>     682                          max_order--;
>     683                  }
>     684
>     685                  p = src ? src + i * PAGE_SIZE : NULL;
>     686                  ret = post_populate(kvm, gfn, pfn, p, max_order, opaque);
>                          ^^^^^^^^^^^^^^^^^^^^
> post_populate() is a pointer to sev_gmem_post_populate() which has is supposed
> to return negative error codes but instead returns number of bytes which failed.
> 
>     687                  if (!ret)
>     688                          kvm_gmem_mark_prepared(folio);
>     689
>     690  put_folio_and_exit:
>     691                  folio_put(folio);
>     692                  if (ret)
>     693                          break;
>     694          }
>     695
>     696          filemap_invalidate_unlock(file->f_mapping);
>     697
>     698          fput(file);
>     699          return ret && !i ? ret : i;
>     700  }
> 
> regards,
> dan carpenter
> 
>