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.
Signed-off-by: Souptick Joarder <[email protected]>
Reviewed-by: Boris Ostrovsky <[email protected]>
---
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
/*
--
1.9.1
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 <[email protected]>
> Reviewed-by: Boris Ostrovsky <[email protected]>
> ---
> 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 Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki
<[email protected]> 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 <[email protected]>
> > Reviewed-by: Boris Ostrovsky <[email protected]>
> > ---
> > 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 <[email protected]> wrote:
>
> On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki
> <[email protected]> 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 <[email protected]>
> > > Reviewed-by: Boris Ostrovsky <[email protected]>
> > > ---
> > > 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 <[email protected]> wrote:
> >
> > On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki
> > <[email protected]> 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 <[email protected]>
> > > > Reviewed-by: Boris Ostrovsky <[email protected]>
> > > > ---
> > > > 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 Mon, Jul 29, 2019 at 7:06 PM Marek Marczykowski-Górecki
<[email protected]> 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 <[email protected]> wrote:
> > >
> > > On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki
> > > <[email protected]> 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 <[email protected]>
> > > > > Reviewed-by: Boris Ostrovsky <[email protected]>
> > > > > ---
> > > > > 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
> <[email protected]> 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 <[email protected]> wrote:
>>>> On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki
>>>> <[email protected]> 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
> > <[email protected]> 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 <[email protected]> wrote:
> >>>> On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki
> >>>> <[email protected]> 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.
--
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 7:52 PM Marek Marczykowski-Górecki
<[email protected]> 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
> > > <[email protected]> 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 <[email protected]> wrote:
> > >>>> On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki
> > >>>> <[email protected]> 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
> <[email protected]> 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
> > > > <[email protected]> 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 <[email protected]> wrote:
> > > >>>> On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki
> > > >>>> <[email protected]> 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!
--
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?