On Thu, May 02, 2019 at 11:28:38AM +0300, Gal Pressman wrote:
> On 01-May-19 19:38, Jason Gunthorpe wrote:
> >> +static int __efa_mmap(struct efa_dev *dev,
> >> + struct efa_ucontext *ucontext,
> >> + struct vm_area_struct *vma,
> >> + struct efa_mmap_entry *entry)
> >> +{
> >> + u8 mmap_flag = get_mmap_flag(entry->key);
> >> + u64 pfn = entry->address >> PAGE_SHIFT;
> >> + u64 address = entry->address;
> >> + u64 length = entry->length;
> >> + unsigned long va;
> >> + int err;
> >> +
> >> + ibdev_dbg(&dev->ibdev,
> >> + "Mapping address[%#llx], length[%#llx], mmap_flag[%d]\n",
> >> + address, length, mmap_flag);
> >> +
> >> + switch (mmap_flag) {
> >> + case EFA_MMAP_IO_NC:
> >> + err = rdma_user_mmap_io(&ucontext->ibucontext, vma, pfn, length,
> >> + pgprot_noncached(vma->vm_page_prot));
> >> + break;
> >> + case EFA_MMAP_IO_WC:
> >> + err = rdma_user_mmap_io(&ucontext->ibucontext, vma, pfn, length,
> >> + pgprot_writecombine(vma->vm_page_prot));
> >> + break;
> >> + case EFA_MMAP_DMA_PAGE:
> >> + for (va = vma->vm_start; va < vma->vm_end;
> >> + va += PAGE_SIZE, pfn++) {
> >> + err = vm_insert_page(vma, va, pfn_to_page(pfn));
> >> + if (err)
> >> + break
> >
> > This loop doesn't bound the number of pfns it accesses, so it is a
> > security problem.
> >
> > The core code was checking this before
>
> Thanks Jason,
> Core code was checking for
> if (vma->vm_end - vma->vm_start != size)
> return ERR_PTR(-EINVAL);
>
> Our code explicitly sets size as 'vma->vm_end - vma->vm_start'.
> In addition, we validate that the mapping size matches the size of the allocated
> buffers which are being mapped (and bounded).
I think it is sketchy to write things like this - pfn is range bound
by entry->size, so that is what should be tested against, not some
indirect inference based on the vma
Jason
