[bug report] KVM: SVM: prevent DBG_DECRYPT and DBG_ENCRYPT overflow

[bug report] KVM: SVM: prevent DBG_DECRYPT and DBG_ENCRYPT overflow

[Dan Carpenter]

Hello David Rientjes,

The patch b86bc2858b38: "KVM: SVM: prevent DBG_DECRYPT and
DBG_ENCRYPT overflow" from Mar 25, 2019, leads to the following
static checker warning:

	arch/x86/kvm/svm/sev.c:960 sev_dbg_crypt()
	error: uninitialized symbol 'ret'.

arch/x86/kvm/svm/sev.c
   879  static int sev_dbg_crypt(struct kvm *kvm, struct kvm_sev_cmd *argp, bool dec)
   880  {
   881          unsigned long vaddr, vaddr_end, next_vaddr;
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

   882          unsigned long dst_vaddr;
                ^^^^^^^^^^^^^^^^^^^^^^^^

These are unsigned long

   883          struct page **src_p, **dst_p;
   884          struct kvm_sev_dbg debug;
   885          unsigned long n;
   886          unsigned int size;
   887          int ret;
   888  
   889          if (!sev_guest(kvm))
   890                  return -ENOTTY;
   891  
   892          if (copy_from_user(&debug, (void __user *)(uintptr_t)argp->data, sizeof(debug)))
   893                  return -EFAULT;
   894  
   895          if (!debug.len || debug.src_uaddr + debug.len < debug.src_uaddr)
                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
But these are u64 so this could still overflow on 32 bit.  Do we care?

   896                  return -EINVAL;
   897          if (!debug.dst_uaddr)
   898                  return -EINVAL;
   899  
   900          vaddr = debug.src_uaddr;
   901          size = debug.len;
   902          vaddr_end = vaddr + size;
   903          dst_vaddr = debug.dst_uaddr;
   904  
   905          for (; vaddr < vaddr_end; vaddr = next_vaddr) {
   906                  int len, s_off, d_off;
   907  
   908                  /* lock userspace source and destination page */
   909                  src_p = sev_pin_memory(kvm, vaddr & PAGE_MASK, PAGE_SIZE, &n, 0);
   910                  if (IS_ERR(src_p))
   911                          return PTR_ERR(src_p);
   912  
   913                  dst_p = sev_pin_memory(kvm, dst_vaddr & PAGE_MASK, PAGE_SIZE, &n, 1);
   914                  if (IS_ERR(dst_p)) {
   915                          sev_unpin_memory(kvm, src_p, n);
   916                          return PTR_ERR(dst_p);
   917                  }
   918  
   919                  /*
   920                   * Flush (on non-coherent CPUs) before DBG_{DE,EN}CRYPT read or modify
   921                   * the pages; flush the destination too so that future accesses do not
   922                   * see stale data.
   923                   */
   924                  sev_clflush_pages(src_p, 1);
   925                  sev_clflush_pages(dst_p, 1);
   926  
   927                  /*
   928                   * Since user buffer may not be page aligned, calculate the
   929                   * offset within the page.
   930                   */
   931                  s_off = vaddr & ~PAGE_MASK;
   932                  d_off = dst_vaddr & ~PAGE_MASK;
   933                  len = min_t(size_t, (PAGE_SIZE - s_off), size);
   934  
   935                  if (dec)
   936                          ret = __sev_dbg_decrypt_user(kvm,
   937                                                       __sme_page_pa(src_p[0]) + s_off,
   938                                                       dst_vaddr,
   939                                                       __sme_page_pa(dst_p[0]) + d_off,
   940                                                       len, &argp->error);
   941                  else
   942                          ret = __sev_dbg_encrypt_user(kvm,
   943                                                       __sme_page_pa(src_p[0]) + s_off,
   944                                                       vaddr,
   945                                                       __sme_page_pa(dst_p[0]) + d_off,
   946                                                       dst_vaddr,
   947                                                       len, &argp->error);
   948  
   949                  sev_unpin_memory(kvm, src_p, n);
   950                  sev_unpin_memory(kvm, dst_p, n);
   951  
   952                  if (ret)
   953                          goto err;
   954  
   955                  next_vaddr = vaddr + len;
   956                  dst_vaddr = dst_vaddr + len;
   957                  size -= len;
   958          }
   959  err:
   960          return ret;
   961  }

regards,
dan carpenter

Re: [bug report] KVM: SVM: prevent DBG_DECRYPT and DBG_ENCRYPT overflow

[Sean Christopherson]

On Thu, Apr 29, 2021, Dan Carpenter wrote:
> Hello David Rientjes,
> 
> The patch b86bc2858b38: "KVM: SVM: prevent DBG_DECRYPT and
> DBG_ENCRYPT overflow" from Mar 25, 2019, leads to the following
> static checker warning:
> 
> 	arch/x86/kvm/svm/sev.c:960 sev_dbg_crypt()
> 	error: uninitialized symbol 'ret'.
> 
> arch/x86/kvm/svm/sev.c
>    879  static int sev_dbg_crypt(struct kvm *kvm, struct kvm_sev_cmd *argp, bool dec)
>    880  {
>    881          unsigned long vaddr, vaddr_end, next_vaddr;
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
>    882          unsigned long dst_vaddr;
>                 ^^^^^^^^^^^^^^^^^^^^^^^^
> 
> These are unsigned long
> 
>    883          struct page **src_p, **dst_p;
>    884          struct kvm_sev_dbg debug;
>    885          unsigned long n;
>    886          unsigned int size;
>    887          int ret;
>    888  
>    889          if (!sev_guest(kvm))
>    890                  return -ENOTTY;
>    891  
>    892          if (copy_from_user(&debug, (void __user *)(uintptr_t)argp->data, sizeof(debug)))
>    893                  return -EFAULT;
>    894  
>    895          if (!debug.len || debug.src_uaddr + debug.len < debug.src_uaddr)
>                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> But these are u64 so this could still overflow on 32 bit.  Do we care?

Not really.  sev_guest() will always be false for CONFIG_KVM_AMD_SEV=n, and
CONFIG_KVM_AMD_SEV is dependent on CONFIG_X86_64=y.  This code is compiled for
32-bit only because everyone has been too lazy to stub out sev.c.