[Xen-devel] [bug report] xen/privcmd: add IOCTL_PRIVCMD_MMAP_RESOURCE

[Dan Carpenter <dan.carpenter@xxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel