[PATCH for-4.11] libs/gnttab: fix FreeBSD gntdev interface

[PATCH for-4.11] libs/gnttab: fix FreeBSD gntdev interface

[Roger Pau Monne]

Current interface to the gntdev in FreeBSD is wrong, and mostly worked
out of luck before the PTI FreeBSD fixes, when kernel and user-space
where sharing the same page tables.

On FreeBSD ioctls have the size of the passed struct encoded in the ioctl
number, because the generic ioctl handler in the OS takes care of
copying the data from user-space to kernel space, and then calls the
device specific ioctl handler. Thus using ioctl structs with variable
sizes is not possible.

The fix is to turn the array of structs at the end of
ioctl_gntdev_alloc_gref and ioctl_gntdev_map_grant_ref into pointers,
that can be properly accessed from the kernel gntdev driver using the
copyin/copyout functions. Note that this is exactly how it's done for
the privcmd driver.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Rationale for acceptance into 4.11:
 - Without this fix the grant table device is not usable on FreeBSD.
 - It affects FreeBSD code exclusively, there's no risk for Linux or
   other OSes.
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Juergen Gross <jgross@suse.com>
---
 tools/include/xen-sys/FreeBSD/gntdev.h |  4 +-
 tools/libs/gnttab/freebsd.c            | 63 +++++++++++++-------------
 2 files changed, 33 insertions(+), 34 deletions(-)

diff --git a/tools/include/xen-sys/FreeBSD/gntdev.h b/tools/include/xen-sys/FreeBSD/gntdev.h
index f3af9a46de..1e39e2f51a 100644
--- a/tools/include/xen-sys/FreeBSD/gntdev.h
+++ b/tools/include/xen-sys/FreeBSD/gntdev.h
@@ -138,7 +138,7 @@ struct ioctl_gntdev_alloc_gref {
     /* OUT parameters */
     uint64_t index;
     /* Variable OUT parameter */
-    uint32_t gref_ids[1];
+    uint32_t *gref_ids;
 };
 
 #define GNTDEV_ALLOC_FLAG_WRITABLE 1
@@ -167,7 +167,7 @@ struct ioctl_gntdev_map_grant_ref {
     /* OUT parameters */
     uint64_t index;
     /* Variable IN parameter */
-    struct ioctl_gntdev_grant_ref refs[1];
+    struct ioctl_gntdev_grant_ref *refs;
 };
 
 #define IOCTL_GNTDEV_UNMAP_GRANT_REF					\
diff --git a/tools/libs/gnttab/freebsd.c b/tools/libs/gnttab/freebsd.c
index 3eaa77235f..5c12fe9b0b 100644
--- a/tools/libs/gnttab/freebsd.c
+++ b/tools/libs/gnttab/freebsd.c
@@ -70,22 +70,21 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 {
     uint32_t i;
     int fd = xgt->fd;
-    struct ioctl_gntdev_map_grant_ref *map;
+    struct ioctl_gntdev_map_grant_ref map;
     void *addr = NULL;
     int domids_stride;
-    unsigned int map_size = ROUNDUP((sizeof(*map) + (count - 1) *
-                                    sizeof(struct ioctl_gntdev_map_grant_ref)),
-                                    PAGE_SHIFT);
+    unsigned int refs_size = ROUNDUP(count *
+                                     sizeof(struct ioctl_gntdev_map_grant_ref),
+                                     PAGE_SHIFT);
 
     domids_stride = (flags & XENGNTTAB_GRANT_MAP_SINGLE_DOMAIN) ? 0 : 1;
-    if ( map_size <= PAGE_SIZE )
-        map = malloc(sizeof(*map) +
-                     (count - 1) * sizeof(struct ioctl_gntdev_map_grant_ref));
+    if ( refs_size <= PAGE_SIZE )
+        map.refs = malloc(refs_size);
     else
     {
-        map = mmap(NULL, map_size, PROT_READ | PROT_WRITE,
-                   MAP_PRIVATE | MAP_ANON, -1, 0);
-        if ( map == MAP_FAILED )
+        map.refs = mmap(NULL, refs_size, PROT_READ | PROT_WRITE,
+                        MAP_PRIVATE | MAP_ANON, -1, 0);
+        if ( map.refs == MAP_FAILED )
         {
             GTERROR(xgt->logger, "anon mmap of map failed");
             return NULL;
@@ -94,26 +93,26 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 
     for ( i = 0; i < count; i++ )
     {
-        map->refs[i].domid = domids[i * domids_stride];
-        map->refs[i].ref = refs[i];
+        map.refs[i].domid = domids[i * domids_stride];
+        map.refs[i].ref = refs[i];
     }
 
-    map->count = count;
+    map.count = count;
 
-    if ( ioctl(fd, IOCTL_GNTDEV_MAP_GRANT_REF, map) )
+    if ( ioctl(fd, IOCTL_GNTDEV_MAP_GRANT_REF, &map) )
     {
         GTERROR(xgt->logger, "ioctl MAP_GRANT_REF failed");
         goto out;
     }
 
     addr = mmap(NULL, PAGE_SIZE * count, prot, MAP_SHARED, fd,
-                map->index);
+                map.index);
     if ( addr != MAP_FAILED )
     {
         int rv = 0;
         struct ioctl_gntdev_unmap_notify notify;
 
-        notify.index = map->index;
+        notify.index = map.index;
         notify.action = 0;
         if ( notify_offset < PAGE_SIZE * count )
         {
@@ -141,7 +140,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 
         /* Unmap the driver slots used to store the grant information. */
         GTERROR(xgt->logger, "mmap failed");
-        unmap_grant.index = map->index;
+        unmap_grant.index = map.index;
         unmap_grant.count = count;
         ioctl(fd, IOCTL_GNTDEV_UNMAP_GRANT_REF, &unmap_grant);
         errno = saved_errno;
@@ -149,10 +148,10 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
     }
 
  out:
-    if ( map_size > PAGE_SIZE )
-        munmap(map, map_size);
+    if ( refs_size > PAGE_SIZE )
+        munmap(map.refs, refs_size);
     else
-        free(map);
+        free(map.refs);
 
     return addr;
 }
@@ -239,16 +238,16 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
     void *area = NULL;
     struct ioctl_gntdev_unmap_notify notify;
     struct ioctl_gntdev_dealloc_gref gref_drop;
-    struct ioctl_gntdev_alloc_gref *gref_info = NULL;
+    struct ioctl_gntdev_alloc_gref gref_info;
 
-    gref_info = malloc(sizeof(*gref_info) + count * sizeof(uint32_t));
-    if ( gref_info == NULL )
+    gref_info.gref_ids = malloc(count * sizeof(uint32_t));
+    if ( gref_info.gref_ids == NULL )
         return NULL;
-    gref_info->domid = domid;
-    gref_info->flags = writable ? GNTDEV_ALLOC_FLAG_WRITABLE : 0;
-    gref_info->count = count;
+    gref_info.domid = domid;
+    gref_info.flags = writable ? GNTDEV_ALLOC_FLAG_WRITABLE : 0;
+    gref_info.count = count;
 
-    err = ioctl(fd, IOCTL_GNTDEV_ALLOC_GREF, gref_info);
+    err = ioctl(fd, IOCTL_GNTDEV_ALLOC_GREF, &gref_info);
     if ( err )
     {
         GSERROR(xgs->logger, "ioctl failed");
@@ -256,7 +255,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
     }
 
     area = mmap(NULL, count * PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
-                fd, gref_info->index);
+                fd, gref_info.index);
 
     if ( area == MAP_FAILED )
     {
@@ -265,7 +264,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
         goto out_remove_fdmap;
     }
 
-    notify.index = gref_info->index;
+    notify.index = gref_info.index;
     notify.action = 0;
     if ( notify_offset < PAGE_SIZE * count )
     {
@@ -286,18 +285,18 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
         area = NULL;
     }
 
-    memcpy(refs, gref_info->gref_ids, count * sizeof(uint32_t));
+    memcpy(refs, gref_info.gref_ids, count * sizeof(uint32_t));
 
  out_remove_fdmap:
     /*
      * Removing the mapping from the file descriptor does not cause the
      * pages to be deallocated until the mapping is removed.
      */
-    gref_drop.index = gref_info->index;
+    gref_drop.index = gref_info.index;
     gref_drop.count = count;
     ioctl(fd, IOCTL_GNTDEV_DEALLOC_GREF, &gref_drop);
  out:
-    free(gref_info);
+    free(gref_info.gref_ids);
 
     return area;
 }
-- 
2.17.0


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

Re: [PATCH for-4.11] libs/gnttab: fix FreeBSD gntdev interface

[Juergen Gross]

On 17/04/18 15:03, Roger Pau Monne wrote:
> Current interface to the gntdev in FreeBSD is wrong, and mostly worked
> out of luck before the PTI FreeBSD fixes, when kernel and user-space
> where sharing the same page tables.
> 
> On FreeBSD ioctls have the size of the passed struct encoded in the ioctl
> number, because the generic ioctl handler in the OS takes care of
> copying the data from user-space to kernel space, and then calls the
> device specific ioctl handler. Thus using ioctl structs with variable
> sizes is not possible.
> 
> The fix is to turn the array of structs at the end of
> ioctl_gntdev_alloc_gref and ioctl_gntdev_map_grant_ref into pointers,
> that can be properly accessed from the kernel gntdev driver using the
> copyin/copyout functions. Note that this is exactly how it's done for
> the privcmd driver.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

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

Re: [PATCH for-4.11] libs/gnttab: fix FreeBSD gntdev interface

[Wei Liu]

On Tue, Apr 17, 2018 at 02:03:41PM +0100, Roger Pau Monne wrote:
> Current interface to the gntdev in FreeBSD is wrong, and mostly worked
> out of luck before the PTI FreeBSD fixes, when kernel and user-space
> where sharing the same page tables.

where -> were?

> 
> On FreeBSD ioctls have the size of the passed struct encoded in the ioctl
> number, because the generic ioctl handler in the OS takes care of
> copying the data from user-space to kernel space, and then calls the
> device specific ioctl handler. Thus using ioctl structs with variable
> sizes is not possible.
> 
> The fix is to turn the array of structs at the end of
> ioctl_gntdev_alloc_gref and ioctl_gntdev_map_grant_ref into pointers,
> that can be properly accessed from the kernel gntdev driver using the
> copyin/copyout functions. Note that this is exactly how it's done for
> the privcmd driver.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Not sure I follow. Isn't turning the array into pointer still results in
a variable length array?

The code looks sane to me.

Wei.

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

Re: [PATCH for-4.11] libs/gnttab: fix FreeBSD gntdev interface

[Roger Pau Monné]

On Thu, Apr 19, 2018 at 09:10:56AM +0100, Wei Liu wrote:
> On Tue, Apr 17, 2018 at 02:03:41PM +0100, Roger Pau Monne wrote:
> > Current interface to the gntdev in FreeBSD is wrong, and mostly worked
> > out of luck before the PTI FreeBSD fixes, when kernel and user-space
> > where sharing the same page tables.
> 
> where -> were?
> 
> > 
> > On FreeBSD ioctls have the size of the passed struct encoded in the ioctl
> > number, because the generic ioctl handler in the OS takes care of
> > copying the data from user-space to kernel space, and then calls the
> > device specific ioctl handler. Thus using ioctl structs with variable
> > sizes is not possible.
> > 
> > The fix is to turn the array of structs at the end of
> > ioctl_gntdev_alloc_gref and ioctl_gntdev_map_grant_ref into pointers,
> > that can be properly accessed from the kernel gntdev driver using the
> > copyin/copyout functions. Note that this is exactly how it's done for
> > the privcmd driver.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Not sure I follow. Isn't turning the array into pointer still results in
> a variable length array?

But it won't be a flexible array member, which is what causes the
issue, it will be an independent pointer. Doing something like:

copyin(kernel_space, user_space, sizeof(struct ioctl));
copyin(krefs_array, kernel_space->refs, sizeof(...));

Will work properly.

The problem with the current layout is that the first copyin is
automatically performed by the ioctl generic system handler, and thus
the sizeof will be wrong because it will use the layout of the struct
as defined in the header, that has only one element in the array.

Thanks, Roger.

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

Re: [PATCH for-4.11] libs/gnttab: fix FreeBSD gntdev interface

[Wei Liu]

On Thu, Apr 19, 2018 at 09:35:44AM +0100, Roger Pau Monné wrote:
> On Thu, Apr 19, 2018 at 09:10:56AM +0100, Wei Liu wrote:
> > On Tue, Apr 17, 2018 at 02:03:41PM +0100, Roger Pau Monne wrote:
> > > Current interface to the gntdev in FreeBSD is wrong, and mostly worked
> > > out of luck before the PTI FreeBSD fixes, when kernel and user-space
> > > where sharing the same page tables.
> > 
> > where -> were?
> > 
> > > 
> > > On FreeBSD ioctls have the size of the passed struct encoded in the ioctl
> > > number, because the generic ioctl handler in the OS takes care of
> > > copying the data from user-space to kernel space, and then calls the
> > > device specific ioctl handler. Thus using ioctl structs with variable
> > > sizes is not possible.
> > > 
> > > The fix is to turn the array of structs at the end of
> > > ioctl_gntdev_alloc_gref and ioctl_gntdev_map_grant_ref into pointers,
> > > that can be properly accessed from the kernel gntdev driver using the
> > > copyin/copyout functions. Note that this is exactly how it's done for
> > > the privcmd driver.
> > > 
> > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > 
> > Not sure I follow. Isn't turning the array into pointer still results in
> > a variable length array?
> 
> But it won't be a flexible array member, which is what causes the
> issue, it will be an independent pointer. Doing something like:
> 
> copyin(kernel_space, user_space, sizeof(struct ioctl));
> copyin(krefs_array, kernel_space->refs, sizeof(...));
> 
> Will work properly.
> 
> The problem with the current layout is that the first copyin is
> automatically performed by the ioctl generic system handler, and thus
> the sizeof will be wrong because it will use the layout of the struct
> as defined in the header, that has only one element in the array.
> 

I see.

Acked-by: Wei Liu <wei.liu2@citrix.com>

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