Re: [Xen-devel] [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages()

[Souptick Joarder <jrdr.linux@xxxxxxxxx>]

On Mon, Jul 29, 2019 at 7:06 PM Marek Marczykowski-Górecki
<marmarek@xxxxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Mon, Jul 29, 2019 at 02:02:54PM +0530, Souptick Joarder wrote:
> > On Mon, Jul 29, 2019 at 1:35 PM Souptick Joarder <jrdr.linux@xxxxxxxxx> 
> > wrote:
> > >
> > > On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki
> > > <marmarek@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > On Fri, Feb 15, 2019 at 08:18:31AM +0530, Souptick Joarder wrote:
> > > > > Convert to use vm_map_pages() to map range of kernel
> > > > > memory to user vma.
> > > > >
> > > > > map->count is passed to vm_map_pages() and internal API
> > > > > verify map->count against count ( count = vma_pages(vma))
> > > > > for page array boundary overrun condition.
> > > >
> > > > This commit breaks gntdev driver. If vma->vm_pgoff > 0, vm_map_pages
> > > > will:
> > > >  - use map->pages starting at vma->vm_pgoff instead of 0
> > >
> > > The actual code ignores vma->vm_pgoff > 0 scenario and mapped
> > > the entire map->pages[i]. Why the entire map->pages[i] needs to be mapped
> > > if vma->vm_pgoff > 0 (in original code) ?
>
> vma->vm_pgoff is used as index passed to gntdev_find_map_index. It's
> basically (ab)using this parameter for "which grant reference to map".
>
> > > are you referring to set vma->vm_pgoff = 0 irrespective of value passed
> > > from user space ? If yes, using vm_map_pages_zero() is an alternate
> > > option.
>
> Yes, that should work.

I prefer to use vm_map_pages_zero() to resolve both the issues. Alternatively
the patch can be reverted as you suggested. Let me know you opinion and wait
for feedback from others.

Boris, would you like to give any feedback ?

>
> > > >  - verify map->count against vma_pages()+vma->vm_pgoff instead of just
> > > >    vma_pages().
> > >
> > > In original code ->
> > >
> > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> > > index 559d4b7f807d..469dfbd6cf90 100644
> > > --- a/drivers/xen/gntdev.c
> > > +++ b/drivers/xen/gntdev.c
> > > @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct
> > > vm_area_struct *vma)
> > > int index = vma->vm_pgoff;
> > > int count = vma_pages(vma);
> > >
> > > Count is user passed value.
> > >
> > > struct gntdev_grant_map *map;
> > > - int i, err = -EINVAL;
> > > + int err = -EINVAL;
> > > if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED))
> > > return -EINVAL;
> > > @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip,
> > > struct vm_area_struct *vma)
> > > goto out_put_map;
> > > if (!use_ptemod) {
> > > - for (i = 0; i < count; i++) {
> > > - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE,
> > > - map->pages[i]);
> > >
> > > and when count > i , we end up with trying to map memory outside
> > > boundary of map->pages[i], which was not correct.
> >
> > typo.
> > s/count > i / count > map->count
>
> gntdev_find_map_index verifies it. Specifically, it looks for a map matching
> both index and count.
>
> > >
> > > - if (err)
> > > - goto out_put_map;
> > > - }
> > > + err = vm_map_pages(vma, map->pages, map->count);
> > > + if (err)
> > > + goto out_put_map;
> > >
> > > With this commit, inside __vm_map_pages(), we have addressed this 
> > > scenario.
> > >
> > > +static int __vm_map_pages(struct vm_area_struct *vma, struct page 
> > > **pages,
> > > + unsigned long num, unsigned long offset)
> > > +{
> > > + unsigned long count = vma_pages(vma);
> > > + unsigned long uaddr = vma->vm_start;
> > > + int ret, i;
> > > +
> > > + /* Fail if the user requested offset is beyond the end of the object */
> > > + if (offset > num)
> > > + return -ENXIO;
> > > +
> > > + /* Fail if the user requested size exceeds available object size */
> > > + if (count > num - offset)
> > > + return -ENXIO;
> > >
> > > By checking count > num -offset. (considering vma->vm_pgoff != 0 as well).
> > > So we will never cross the boundary of map->pages[i].
> > >
> > >
> > > >
> > > > In practice, this breaks using a single gntdev FD for mapping multiple
> > > > grants.
> > >
> > > How ?
>
> gntdev uses vma->vm_pgoff to select which grant entry should be mapped.
> map struct returned by gntdev_find_map_index() describes just the pages
> to be mapped. Specifically map->pages[0] should be mapped at
> vma->vm_start, not vma->vm_start+vma->vm_pgoff*PAGE_SIZE.
>
> When trying to map grant with index (aka vma->vm_pgoff) > 1,
> __vm_map_pages() will refuse to map it because it will expect map->count
> to be at least vma_pages(vma)+vma->vm_pgoff, while it is exactly
> vma_pages(vma).
>
> > > > It looks like vm_map_pages() is not a good fit for this code and IMO it
> > > > should be reverted.
> > >
> > > Did you hit any issue around this code in real time ?
>
> Yes, relevant strace output:
> [pid   857] ioctl(7, IOCTL_GNTDEV_MAP_GRANT_REF, 0x7ffd3407b6d0) = 0
> [pid   857] mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_SHARED, 7, 0) = 
> 0x777f1211b000
> [pid   857] ioctl(7, IOCTL_GNTDEV_SET_UNMAP_NOTIFY, 0x7ffd3407b710) = 0
> [pid   857] ioctl(7, IOCTL_GNTDEV_MAP_GRANT_REF, 0x7ffd3407b6d0) = 0
> [pid   857] mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_SHARED, 7, 0x1000) = 
> -1 ENXIO (No such device or address)
>
> details here:
> https://github.com/QubesOS/qubes-issues/issues/5199
>
>
> > >
> > >
> > > >
> > > > > Signed-off-by: Souptick Joarder <jrdr.linux@xxxxxxxxx>
> > > > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> > > > > ---
> > > > >  drivers/xen/gntdev.c | 11 ++++-------
> > > > >  1 file changed, 4 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> > > > > index 5efc5ee..5d64262 100644
> > > > > --- a/drivers/xen/gntdev.c
> > > > > +++ b/drivers/xen/gntdev.c
> > > > > @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, 
> > > > > struct vm_area_struct *vma)
> > > > >       int index = vma->vm_pgoff;
> > > > >       int count = vma_pages(vma);
> > > > >       struct gntdev_grant_map *map;
> > > > > -     int i, err = -EINVAL;
> > > > > +     int err = -EINVAL;
> > > > >
> > > > >       if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED))
> > > > >               return -EINVAL;
> > > > > @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, 
> > > > > struct vm_area_struct *vma)
> > > > >               goto out_put_map;
> > > > >
> > > > >       if (!use_ptemod) {
> > > > > -             for (i = 0; i < count; i++) {
> > > > > -                     err = vm_insert_page(vma, vma->vm_start + 
> > > > > i*PAGE_SIZE,
> > > > > -                             map->pages[i]);
> > > > > -                     if (err)
> > > > > -                             goto out_put_map;
> > > > > -             }
> > > > > +             err = vm_map_pages(vma, map->pages, map->count);
> > > > > +             if (err)
> > > > > +                     goto out_put_map;
> > > > >       } else {
> > > > >  #ifdef CONFIG_X86
> > > > >               /*
> > > >
> > > > --
> > > > Best Regards,
> > > > Marek Marczykowski-Górecki
> > > > Invisible Things Lab
> > > > A: Because it messes up the order in which people normally read text.
> > > > Q: Why is top-posting such a bad thing?
>
> --
> Best Regards,
> Marek Marczykowski-Górecki
> Invisible Things Lab
> A: Because it messes up the order in which people normally read text.
> Q: Why is top-posting such a bad thing?

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