| Message ID | 20190215024830.GA26477@jordon-HP-15-Notebook-PC (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: Use vm_map_pages() and vm_map_pages_zero() API | expand |
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 - verify map->count against vma_pages()+vma->vm_pgoff instead of just vma_pages(). In practice, this breaks using a single gntdev FD for mapping multiple grants. It looks like vm_map_pages() is not a good fit for this code and IMO it should be reverted. > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > --- > 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 > /*
On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> 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) ? 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. > - 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. - 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 ? > > 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 ? > > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > --- > > 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?
On Mon, Jul 29, 2019 at 1:35 PM Souptick Joarder <jrdr.linux@gmail.com> wrote: > > On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki > <marmarek@invisiblethingslab.com> 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) ? > > 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. > > > > - 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 > > - 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 ? > > > > > 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 ? > > > > > > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > > --- > > > 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?
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@gmail.com> wrote: > > > > On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki > > <marmarek@invisiblethingslab.com> 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. > > > - 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@gmail.com> > > > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > > > --- > > > > 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?
On Mon, Jul 29, 2019 at 7:06 PM Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> 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@gmail.com> wrote: > > > > > > On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki > > > <marmarek@invisiblethingslab.com> 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@gmail.com> > > > > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > > > > --- > > > > > 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?
On 7/30/19 2:03 AM, Souptick Joarder wrote: > On Mon, Jul 29, 2019 at 7:06 PM Marek Marczykowski-Górecki > <marmarek@invisiblethingslab.com> 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@gmail.com> wrote: >>>> On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki >>>> <marmarek@invisiblethingslab.com> 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 ? vm_map_pages_zero() looks good to me. Marek, does it work for you? -boris
On Tue, Jul 30, 2019 at 10:05:42AM -0400, Boris Ostrovsky wrote: > On 7/30/19 2:03 AM, Souptick Joarder wrote: > > On Mon, Jul 29, 2019 at 7:06 PM Marek Marczykowski-Górecki > > <marmarek@invisiblethingslab.com> 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@gmail.com> wrote: > >>>> On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki > >>>> <marmarek@invisiblethingslab.com> 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 ? > > vm_map_pages_zero() looks good to me. Marek, does it work for you? Yes, replacing vm_map_pages() with vm_map_pages_zero() fixes the problem for me.
On Tue, Jul 30, 2019 at 7:52 PM Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> wrote: > > On Tue, Jul 30, 2019 at 10:05:42AM -0400, Boris Ostrovsky wrote: > > On 7/30/19 2:03 AM, Souptick Joarder wrote: > > > On Mon, Jul 29, 2019 at 7:06 PM Marek Marczykowski-Górecki > > > <marmarek@invisiblethingslab.com> 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@gmail.com> wrote: > > >>>> On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki > > >>>> <marmarek@invisiblethingslab.com> 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 ? > > > > vm_map_pages_zero() looks good to me. Marek, does it work for you? > > Yes, replacing vm_map_pages() with vm_map_pages_zero() fixes the > problem for me. Marek, I can send a patch for the same if you are ok. We need to cc stable as this changes are available in 5.2.4. > > -- > 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?
On Tue, Jul 30, 2019 at 08:22:02PM +0530, Souptick Joarder wrote: > On Tue, Jul 30, 2019 at 7:52 PM Marek Marczykowski-Górecki > <marmarek@invisiblethingslab.com> wrote: > > > > On Tue, Jul 30, 2019 at 10:05:42AM -0400, Boris Ostrovsky wrote: > > > On 7/30/19 2:03 AM, Souptick Joarder wrote: > > > > On Mon, Jul 29, 2019 at 7:06 PM Marek Marczykowski-Górecki > > > > <marmarek@invisiblethingslab.com> 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@gmail.com> wrote: > > > >>>> On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki > > > >>>> <marmarek@invisiblethingslab.com> 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 ? > > > > > > vm_map_pages_zero() looks good to me. Marek, does it work for you? > > > > Yes, replacing vm_map_pages() with vm_map_pages_zero() fixes the > > problem for me. > > Marek, I can send a patch for the same if you are ok. > We need to cc stable as this changes are available in 5.2.4. Sounds good, thanks!
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 /*