| Message ID | 20240612115040.2423290-4-dan.carpenter@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] KVM: SVM: Fix uninitialized variable bug | expand |
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), ¶ms, 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 >
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
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 > >
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); }
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(-)