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

[Dan Carpenter <dan.carpenter@linaro.org>]

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