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

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

[Souptick Joarder]

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 <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
 		/*
-- 
1.9.1

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

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 2088 bytes --]

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
>  		/*

-- 
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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Souptick Joarder]

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?

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

[Souptick Joarder]

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?

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

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 6813 bytes --]

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?

-- 
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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Souptick Joarder]

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?

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

[Boris Ostrovsky]

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

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

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 2180 bytes --]

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.

-- 
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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Souptick Joarder]

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?

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

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 2658 bytes --]

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!

-- 
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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]