All of lore.kernel.org
 help / color / mirror / Atom feed
* [bug report] xen/privcmd: add IOCTL_PRIVCMD_MMAP_RESOURCE
@ 2018-06-07  8:32 Dan Carpenter
  2018-06-07  8:43 ` Paul Durrant
  0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2018-06-07  8:32 UTC (permalink / raw)
  To: paul.durrant; +Cc: xen-devel

Hello Paul Durrant,

The patch 3ad0876554ca: "xen/privcmd: add
IOCTL_PRIVCMD_MMAP_RESOURCE" from May 9, 2018, leads to the following
static checker warning:

	drivers/xen/privcmd.c:827 privcmd_ioctl_mmap_resource()
	warn: passing casted pointer 'pfns' to 'xen_remap_domain_mfn_array()' 64 vs 32.

drivers/xen/privcmd.c
   746  static long privcmd_ioctl_mmap_resource(struct file *file, void __user *udata)
   747  {
   748          struct privcmd_data *data = file->private_data;
   749          struct mm_struct *mm = current->mm;
   750          struct vm_area_struct *vma;
   751          struct privcmd_mmap_resource kdata;
   752          xen_pfn_t *pfns = NULL;
   753          struct xen_mem_acquire_resource xdata;
   754          int rc;
   755  
   756          if (copy_from_user(&kdata, udata, sizeof(kdata)))
   757                  return -EFAULT;
   758  
   759          /* If restriction is in place, check the domid matches */
   760          if (data->domid != DOMID_INVALID && data->domid != kdata.dom)
   761                  return -EPERM;
   762  
   763          down_write(&mm->mmap_sem);
   764  
   765          vma = find_vma(mm, kdata.addr);
   766          if (!vma || vma->vm_ops != &privcmd_vm_ops) {
   767                  rc = -EINVAL;
   768                  goto out;
   769          }
   770  
   771          pfns = kcalloc(kdata.num, sizeof(*pfns), GFP_KERNEL);
   772          if (!pfns) {
   773                  rc = -ENOMEM;
   774                  goto out;
   775          }
   776  
   777          if (xen_feature(XENFEAT_auto_translated_physmap)) {
   778                  unsigned int nr = DIV_ROUND_UP(kdata.num, XEN_PFN_PER_PAGE);
   779                  struct page **pages;
   780                  unsigned int i;
   781  
   782                  rc = alloc_empty_pages(vma, nr);
   783                  if (rc < 0)
   784                          goto out;
   785  
   786                  pages = vma->vm_private_data;
   787                  for (i = 0; i < kdata.num; i++) {
   788                          xen_pfn_t pfn =
   789                                  page_to_xen_pfn(pages[i / XEN_PFN_PER_PAGE]);
   790  
   791                          pfns[i] = pfn + (i % XEN_PFN_PER_PAGE);
   792                  }
   793          } else
   794                  vma->vm_private_data = PRIV_VMA_LOCKED;
   795  
   796          memset(&xdata, 0, sizeof(xdata));
   797          xdata.domid = kdata.dom;
   798          xdata.type = kdata.type;
   799          xdata.id = kdata.id;
   800          xdata.frame = kdata.idx;
   801          xdata.nr_frames = kdata.num;
   802          set_xen_guest_handle(xdata.frame_list, pfns);
   803  
   804          xen_preemptible_hcall_begin();
   805          rc = HYPERVISOR_memory_op(XENMEM_acquire_resource, &xdata);
   806          xen_preemptible_hcall_end();
   807  
   808          if (rc)
   809                  goto out;
   810  
   811          if (xen_feature(XENFEAT_auto_translated_physmap)) {
   812                  struct remap_pfn r = {
   813                          .mm = vma->vm_mm,
   814                          .pages = vma->vm_private_data,
   815                          .prot = vma->vm_page_prot,
   816                  };
   817  
   818                  rc = apply_to_page_range(r.mm, kdata.addr,
   819                                           kdata.num << PAGE_SHIFT,
   820                                           remap_pfn_fn, &r);
   821          } else {
   822                  unsigned int domid =
   823                          (xdata.flags & XENMEM_rsrc_acq_caller_owned) ?
   824                          DOMID_SELF : kdata.dom;
   825                  int num;
   826  
   827                  num = xen_remap_domain_mfn_array(vma,
   828                                                   kdata.addr & PAGE_MASK,
   829                                                   pfns, kdata.num, (int *)pfns,
                                                         ^^^^             ^^^^^^^^^^^
I'm a newbie to this code, but I don't understand how it can be right...
The (int *)pfns argument is used to store integer error codes, but that
will only write to the first half of the buffer?  Unless this only works
on 32 bit CPUs?

   830                                                   vma->vm_page_prot,
   831                                                   domid,
   832                                                   vma->vm_private_data);
   833                  if (num < 0)
   834                          rc = num;
   835                  else if (num != kdata.num) {
   836                          unsigned int i;
   837  
   838                          for (i = 0; i < num; i++) {
   839                                  rc = pfns[i];
   840                                  if (rc < 0)
   841                                          break;
   842                          }
   843                  } else
   844                          rc = 0;
   845          }
   846  
   847  out:
   848          up_write(&mm->mmap_sem);
   849          kfree(pfns);
   850  
   851          return rc;
   852  }

regards,
dan carpenter

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [bug report] xen/privcmd: add IOCTL_PRIVCMD_MMAP_RESOURCE
  2018-06-07  8:32 [bug report] xen/privcmd: add IOCTL_PRIVCMD_MMAP_RESOURCE Dan Carpenter
@ 2018-06-07  8:43 ` Paul Durrant
  0 siblings, 0 replies; 2+ messages in thread
From: Paul Durrant @ 2018-06-07  8:43 UTC (permalink / raw)
  To: 'Dan Carpenter'; +Cc: xen-devel

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: 07 June 2018 09:32
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org
> Subject: [bug report] xen/privcmd: add IOCTL_PRIVCMD_MMAP_RESOURCE
> 
> Hello Paul Durrant,
> 
> The patch 3ad0876554ca: "xen/privcmd: add
> IOCTL_PRIVCMD_MMAP_RESOURCE" from May 9, 2018, leads to the
> following
> static checker warning:
> 
> 	drivers/xen/privcmd.c:827 privcmd_ioctl_mmap_resource()
> 	warn: passing casted pointer 'pfns' to
> 'xen_remap_domain_mfn_array()' 64 vs 32.
> 
> drivers/xen/privcmd.c
>    746  static long privcmd_ioctl_mmap_resource(struct file *file, void __user
> *udata)
>    747  {
>    748          struct privcmd_data *data = file->private_data;
>    749          struct mm_struct *mm = current->mm;
>    750          struct vm_area_struct *vma;
>    751          struct privcmd_mmap_resource kdata;
>    752          xen_pfn_t *pfns = NULL;
>    753          struct xen_mem_acquire_resource xdata;
>    754          int rc;
>    755
>    756          if (copy_from_user(&kdata, udata, sizeof(kdata)))
>    757                  return -EFAULT;
>    758
>    759          /* If restriction is in place, check the domid matches */
>    760          if (data->domid != DOMID_INVALID && data->domid !=
> kdata.dom)
>    761                  return -EPERM;
>    762
>    763          down_write(&mm->mmap_sem);
>    764
>    765          vma = find_vma(mm, kdata.addr);
>    766          if (!vma || vma->vm_ops != &privcmd_vm_ops) {
>    767                  rc = -EINVAL;
>    768                  goto out;
>    769          }
>    770
>    771          pfns = kcalloc(kdata.num, sizeof(*pfns), GFP_KERNEL);
>    772          if (!pfns) {
>    773                  rc = -ENOMEM;
>    774                  goto out;
>    775          }
>    776
>    777          if (xen_feature(XENFEAT_auto_translated_physmap)) {
>    778                  unsigned int nr = DIV_ROUND_UP(kdata.num,
> XEN_PFN_PER_PAGE);
>    779                  struct page **pages;
>    780                  unsigned int i;
>    781
>    782                  rc = alloc_empty_pages(vma, nr);
>    783                  if (rc < 0)
>    784                          goto out;
>    785
>    786                  pages = vma->vm_private_data;
>    787                  for (i = 0; i < kdata.num; i++) {
>    788                          xen_pfn_t pfn =
>    789                                  page_to_xen_pfn(pages[i / XEN_PFN_PER_PAGE]);
>    790
>    791                          pfns[i] = pfn + (i % XEN_PFN_PER_PAGE);
>    792                  }
>    793          } else
>    794                  vma->vm_private_data = PRIV_VMA_LOCKED;
>    795
>    796          memset(&xdata, 0, sizeof(xdata));
>    797          xdata.domid = kdata.dom;
>    798          xdata.type = kdata.type;
>    799          xdata.id = kdata.id;
>    800          xdata.frame = kdata.idx;
>    801          xdata.nr_frames = kdata.num;
>    802          set_xen_guest_handle(xdata.frame_list, pfns);
>    803
>    804          xen_preemptible_hcall_begin();
>    805          rc = HYPERVISOR_memory_op(XENMEM_acquire_resource,
> &xdata);
>    806          xen_preemptible_hcall_end();
>    807
>    808          if (rc)
>    809                  goto out;
>    810
>    811          if (xen_feature(XENFEAT_auto_translated_physmap)) {
>    812                  struct remap_pfn r = {
>    813                          .mm = vma->vm_mm,
>    814                          .pages = vma->vm_private_data,
>    815                          .prot = vma->vm_page_prot,
>    816                  };
>    817
>    818                  rc = apply_to_page_range(r.mm, kdata.addr,
>    819                                           kdata.num << PAGE_SHIFT,
>    820                                           remap_pfn_fn, &r);
>    821          } else {
>    822                  unsigned int domid =
>    823                          (xdata.flags & XENMEM_rsrc_acq_caller_owned) ?
>    824                          DOMID_SELF : kdata.dom;
>    825                  int num;
>    826
>    827                  num = xen_remap_domain_mfn_array(vma,
>    828                                                   kdata.addr & PAGE_MASK,
>    829                                                   pfns, kdata.num, (int *)pfns,
>                                                          ^^^^             ^^^^^^^^^^^
> I'm a newbie to this code, but I don't understand how it can be right...
> The (int *)pfns argument is used to store integer error codes, but that
> will only write to the first half of the buffer?  Unless this only works
> on 32 bit CPUs?

No, it's supposed to work on 64 bit too and that is a very good point. The cast is still fine since the buffer re-use is safe but it needs to be correctly cast...

> 
>    830                                                   vma->vm_page_prot,
>    831                                                   domid,
>    832                                                   vma->vm_private_data);
>    833                  if (num < 0)
>    834                          rc = num;
>    835                  else if (num != kdata.num) {
>    836                          unsigned int i;
>    837
>    838                          for (i = 0; i < num; i++) {
>    839                                  rc = pfns[i];

...here too.

TBH though I think I'll just allocate a separate array. I never liked this scheme, I was just following previous style.

Thanks for pointing this out,

  Paul

>    840                                  if (rc < 0)
>    841                                          break;
>    842                          }
>    843                  } else
>    844                          rc = 0;
>    845          }
>    846
>    847  out:
>    848          up_write(&mm->mmap_sem);
>    849          kfree(pfns);
>    850
>    851          return rc;
>    852  }
> 
> regards,
> dan carpenter

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2018-06-07  8:43 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-07  8:32 [bug report] xen/privcmd: add IOCTL_PRIVCMD_MMAP_RESOURCE Dan Carpenter
2018-06-07  8:43 ` Paul Durrant

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.