[Qemu-devel] [PATCH] xen/disk: don't leak stack data via response ring

[Qemu-devel] [PATCH] xen/disk: don't leak stack data via response ring

[Jan Beulich]

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

Rather than constructing a local structure instance on the stack, fill
the fields directly on the shared ring, just like other (Linux)
backends do. Build on the fact that all response structure flavors are
actually identical (the old code did make this assumption too).

This is XSA-216.

Reported by: Anthony Perard <anthony.perard@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
---
v2: Add QEMU_PACKED to fix handling 32-bit guests by 64-bit qemu.

--- a/hw/block/xen_blkif.h
+++ b/hw/block/xen_blkif.h
@@ -14,9 +14,6 @@
 struct blkif_common_request {
     char dummy;
 };
-struct blkif_common_response {
-    char dummy;
-};
 
 /* i386 protocol version */
 #pragma pack(push, 4)
@@ -36,13 +33,7 @@ struct blkif_x86_32_request_discard {
     blkif_sector_t sector_number;    /* start sector idx on disk (r/w only)  */
     uint64_t       nr_sectors;       /* # of contiguous sectors to discard   */
 };
-struct blkif_x86_32_response {
-    uint64_t        id;              /* copied from request */
-    uint8_t         operation;       /* copied from request */
-    int16_t         status;          /* BLKIF_RSP_???       */
-};
 typedef struct blkif_x86_32_request blkif_x86_32_request_t;
-typedef struct blkif_x86_32_response blkif_x86_32_response_t;
 #pragma pack(pop)
 
 /* x86_64 protocol version */
@@ -62,20 +53,14 @@ struct blkif_x86_64_request_discard {
     blkif_sector_t sector_number;    /* start sector idx on disk (r/w only)  */
     uint64_t       nr_sectors;       /* # of contiguous sectors to discard   */
 };
-struct blkif_x86_64_response {
-    uint64_t       __attribute__((__aligned__(8))) id;
-    uint8_t         operation;       /* copied from request */
-    int16_t         status;          /* BLKIF_RSP_???       */
-};
 typedef struct blkif_x86_64_request blkif_x86_64_request_t;
-typedef struct blkif_x86_64_response blkif_x86_64_response_t;
 
 DEFINE_RING_TYPES(blkif_common, struct blkif_common_request,
-                  struct blkif_common_response);
+                  struct blkif_response);
 DEFINE_RING_TYPES(blkif_x86_32, struct blkif_x86_32_request,
-                  struct blkif_x86_32_response);
+                  struct blkif_response QEMU_PACKED);
 DEFINE_RING_TYPES(blkif_x86_64, struct blkif_x86_64_request,
-                  struct blkif_x86_64_response);
+                  struct blkif_response);
 
 union blkif_back_rings {
     blkif_back_ring_t        native;
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -769,31 +769,30 @@ static int blk_send_response_one(struct
     struct XenBlkDev  *blkdev = ioreq->blkdev;
     int               send_notify   = 0;
     int               have_requests = 0;
-    blkif_response_t  resp;
-    void              *dst;
-
-    resp.id        = ioreq->req.id;
-    resp.operation = ioreq->req.operation;
-    resp.status    = ioreq->status;
+    blkif_response_t  *resp;
 
     /* Place on the response ring for the relevant domain. */
     switch (blkdev->protocol) {
     case BLKIF_PROTOCOL_NATIVE:
-        dst = RING_GET_RESPONSE(&blkdev->rings.native, blkdev->rings.native.rsp_prod_pvt);
+        resp = RING_GET_RESPONSE(&blkdev->rings.native,
+                                 blkdev->rings.native.rsp_prod_pvt);
         break;
     case BLKIF_PROTOCOL_X86_32:
-        dst = RING_GET_RESPONSE(&blkdev->rings.x86_32_part,
-                                blkdev->rings.x86_32_part.rsp_prod_pvt);
+        resp = RING_GET_RESPONSE(&blkdev->rings.x86_32_part,
+                                 blkdev->rings.x86_32_part.rsp_prod_pvt);
         break;
     case BLKIF_PROTOCOL_X86_64:
-        dst = RING_GET_RESPONSE(&blkdev->rings.x86_64_part,
-                                blkdev->rings.x86_64_part.rsp_prod_pvt);
+        resp = RING_GET_RESPONSE(&blkdev->rings.x86_64_part,
+                                 blkdev->rings.x86_64_part.rsp_prod_pvt);
         break;
     default:
-        dst = NULL;
         return 0;
     }
-    memcpy(dst, &resp, sizeof(resp));
+
+    resp->id        = ioreq->req.id;
+    resp->operation = ioreq->req.operation;
+    resp->status    = ioreq->status;
+
     blkdev->rings.common.rsp_prod_pvt++;
 
     RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&blkdev->rings.common, send_notify);



[-- Attachment #2: xsa216-qemuu.patch --]
[-- Type: text/plain, Size: 4520 bytes --]

xen/disk: don't leak stack data via response ring

Rather than constructing a local structure instance on the stack, fill
the fields directly on the shared ring, just like other (Linux)
backends do. Build on the fact that all response structure flavors are
actually identical (the old code did make this assumption too).

This is XSA-216.

Reported by: Anthony Perard <anthony.perard@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
---
v2: Add QEMU_PACKED to fix handling 32-bit guests by 64-bit qemu.

--- a/hw/block/xen_blkif.h
+++ b/hw/block/xen_blkif.h
@@ -14,9 +14,6 @@
 struct blkif_common_request {
     char dummy;
 };
-struct blkif_common_response {
-    char dummy;
-};
 
 /* i386 protocol version */
 #pragma pack(push, 4)
@@ -36,13 +33,7 @@ struct blkif_x86_32_request_discard {
     blkif_sector_t sector_number;    /* start sector idx on disk (r/w only)  */
     uint64_t       nr_sectors;       /* # of contiguous sectors to discard   */
 };
-struct blkif_x86_32_response {
-    uint64_t        id;              /* copied from request */
-    uint8_t         operation;       /* copied from request */
-    int16_t         status;          /* BLKIF_RSP_???       */
-};
 typedef struct blkif_x86_32_request blkif_x86_32_request_t;
-typedef struct blkif_x86_32_response blkif_x86_32_response_t;
 #pragma pack(pop)
 
 /* x86_64 protocol version */
@@ -62,20 +53,14 @@ struct blkif_x86_64_request_discard {
     blkif_sector_t sector_number;    /* start sector idx on disk (r/w only)  */
     uint64_t       nr_sectors;       /* # of contiguous sectors to discard   */
 };
-struct blkif_x86_64_response {
-    uint64_t       __attribute__((__aligned__(8))) id;
-    uint8_t         operation;       /* copied from request */
-    int16_t         status;          /* BLKIF_RSP_???       */
-};
 typedef struct blkif_x86_64_request blkif_x86_64_request_t;
-typedef struct blkif_x86_64_response blkif_x86_64_response_t;
 
 DEFINE_RING_TYPES(blkif_common, struct blkif_common_request,
-                  struct blkif_common_response);
+                  struct blkif_response);
 DEFINE_RING_TYPES(blkif_x86_32, struct blkif_x86_32_request,
-                  struct blkif_x86_32_response);
+                  struct blkif_response QEMU_PACKED);
 DEFINE_RING_TYPES(blkif_x86_64, struct blkif_x86_64_request,
-                  struct blkif_x86_64_response);
+                  struct blkif_response);
 
 union blkif_back_rings {
     blkif_back_ring_t        native;
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -769,31 +769,30 @@ static int blk_send_response_one(struct
     struct XenBlkDev  *blkdev = ioreq->blkdev;
     int               send_notify   = 0;
     int               have_requests = 0;
-    blkif_response_t  resp;
-    void              *dst;
-
-    resp.id        = ioreq->req.id;
-    resp.operation = ioreq->req.operation;
-    resp.status    = ioreq->status;
+    blkif_response_t  *resp;
 
     /* Place on the response ring for the relevant domain. */
     switch (blkdev->protocol) {
     case BLKIF_PROTOCOL_NATIVE:
-        dst = RING_GET_RESPONSE(&blkdev->rings.native, blkdev->rings.native.rsp_prod_pvt);
+        resp = RING_GET_RESPONSE(&blkdev->rings.native,
+                                 blkdev->rings.native.rsp_prod_pvt);
         break;
     case BLKIF_PROTOCOL_X86_32:
-        dst = RING_GET_RESPONSE(&blkdev->rings.x86_32_part,
-                                blkdev->rings.x86_32_part.rsp_prod_pvt);
+        resp = RING_GET_RESPONSE(&blkdev->rings.x86_32_part,
+                                 blkdev->rings.x86_32_part.rsp_prod_pvt);
         break;
     case BLKIF_PROTOCOL_X86_64:
-        dst = RING_GET_RESPONSE(&blkdev->rings.x86_64_part,
-                                blkdev->rings.x86_64_part.rsp_prod_pvt);
+        resp = RING_GET_RESPONSE(&blkdev->rings.x86_64_part,
+                                 blkdev->rings.x86_64_part.rsp_prod_pvt);
         break;
     default:
-        dst = NULL;
         return 0;
     }
-    memcpy(dst, &resp, sizeof(resp));
+
+    resp->id        = ioreq->req.id;
+    resp->operation = ioreq->req.operation;
+    resp->status    = ioreq->status;
+
     blkdev->rings.common.rsp_prod_pvt++;
 
     RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&blkdev->rings.common, send_notify);

[PATCH] xen/disk: don't leak stack data via response ring

[Jan Beulich]

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

Rather than constructing a local structure instance on the stack, fill
the fields directly on the shared ring, just like other (Linux)
backends do. Build on the fact that all response structure flavors are
actually identical (the old code did make this assumption too).

This is XSA-216.

Reported by: Anthony Perard <anthony.perard@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
---
v2: Add QEMU_PACKED to fix handling 32-bit guests by 64-bit qemu.

--- a/hw/block/xen_blkif.h
+++ b/hw/block/xen_blkif.h
@@ -14,9 +14,6 @@
 struct blkif_common_request {
     char dummy;
 };
-struct blkif_common_response {
-    char dummy;
-};
 
 /* i386 protocol version */
 #pragma pack(push, 4)
@@ -36,13 +33,7 @@ struct blkif_x86_32_request_discard {
     blkif_sector_t sector_number;    /* start sector idx on disk (r/w only)  */
     uint64_t       nr_sectors;       /* # of contiguous sectors to discard   */
 };
-struct blkif_x86_32_response {
-    uint64_t        id;              /* copied from request */
-    uint8_t         operation;       /* copied from request */
-    int16_t         status;          /* BLKIF_RSP_???       */
-};
 typedef struct blkif_x86_32_request blkif_x86_32_request_t;
-typedef struct blkif_x86_32_response blkif_x86_32_response_t;
 #pragma pack(pop)
 
 /* x86_64 protocol version */
@@ -62,20 +53,14 @@ struct blkif_x86_64_request_discard {
     blkif_sector_t sector_number;    /* start sector idx on disk (r/w only)  */
     uint64_t       nr_sectors;       /* # of contiguous sectors to discard   */
 };
-struct blkif_x86_64_response {
-    uint64_t       __attribute__((__aligned__(8))) id;
-    uint8_t         operation;       /* copied from request */
-    int16_t         status;          /* BLKIF_RSP_???       */
-};
 typedef struct blkif_x86_64_request blkif_x86_64_request_t;
-typedef struct blkif_x86_64_response blkif_x86_64_response_t;
 
 DEFINE_RING_TYPES(blkif_common, struct blkif_common_request,
-                  struct blkif_common_response);
+                  struct blkif_response);
 DEFINE_RING_TYPES(blkif_x86_32, struct blkif_x86_32_request,
-                  struct blkif_x86_32_response);
+                  struct blkif_response QEMU_PACKED);
 DEFINE_RING_TYPES(blkif_x86_64, struct blkif_x86_64_request,
-                  struct blkif_x86_64_response);
+                  struct blkif_response);
 
 union blkif_back_rings {
     blkif_back_ring_t        native;
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -769,31 +769,30 @@ static int blk_send_response_one(struct
     struct XenBlkDev  *blkdev = ioreq->blkdev;
     int               send_notify   = 0;
     int               have_requests = 0;
-    blkif_response_t  resp;
-    void              *dst;
-
-    resp.id        = ioreq->req.id;
-    resp.operation = ioreq->req.operation;
-    resp.status    = ioreq->status;
+    blkif_response_t  *resp;
 
     /* Place on the response ring for the relevant domain. */
     switch (blkdev->protocol) {
     case BLKIF_PROTOCOL_NATIVE:
-        dst = RING_GET_RESPONSE(&blkdev->rings.native, blkdev->rings.native.rsp_prod_pvt);
+        resp = RING_GET_RESPONSE(&blkdev->rings.native,
+                                 blkdev->rings.native.rsp_prod_pvt);
         break;
     case BLKIF_PROTOCOL_X86_32:
-        dst = RING_GET_RESPONSE(&blkdev->rings.x86_32_part,
-                                blkdev->rings.x86_32_part.rsp_prod_pvt);
+        resp = RING_GET_RESPONSE(&blkdev->rings.x86_32_part,
+                                 blkdev->rings.x86_32_part.rsp_prod_pvt);
         break;
     case BLKIF_PROTOCOL_X86_64:
-        dst = RING_GET_RESPONSE(&blkdev->rings.x86_64_part,
-                                blkdev->rings.x86_64_part.rsp_prod_pvt);
+        resp = RING_GET_RESPONSE(&blkdev->rings.x86_64_part,
+                                 blkdev->rings.x86_64_part.rsp_prod_pvt);
         break;
     default:
-        dst = NULL;
         return 0;
     }
-    memcpy(dst, &resp, sizeof(resp));
+
+    resp->id        = ioreq->req.id;
+    resp->operation = ioreq->req.operation;
+    resp->status    = ioreq->status;
+
     blkdev->rings.common.rsp_prod_pvt++;
 
     RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&blkdev->rings.common, send_notify);



[-- Attachment #2: xsa216-qemuu.patch --]
[-- Type: text/plain, Size: 4520 bytes --]

xen/disk: don't leak stack data via response ring

Rather than constructing a local structure instance on the stack, fill
the fields directly on the shared ring, just like other (Linux)
backends do. Build on the fact that all response structure flavors are
actually identical (the old code did make this assumption too).

This is XSA-216.

Reported by: Anthony Perard <anthony.perard@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
---
v2: Add QEMU_PACKED to fix handling 32-bit guests by 64-bit qemu.

--- a/hw/block/xen_blkif.h
+++ b/hw/block/xen_blkif.h
@@ -14,9 +14,6 @@
 struct blkif_common_request {
     char dummy;
 };
-struct blkif_common_response {
-    char dummy;
-};
 
 /* i386 protocol version */
 #pragma pack(push, 4)
@@ -36,13 +33,7 @@ struct blkif_x86_32_request_discard {
     blkif_sector_t sector_number;    /* start sector idx on disk (r/w only)  */
     uint64_t       nr_sectors;       /* # of contiguous sectors to discard   */
 };
-struct blkif_x86_32_response {
-    uint64_t        id;              /* copied from request */
-    uint8_t         operation;       /* copied from request */
-    int16_t         status;          /* BLKIF_RSP_???       */
-};
 typedef struct blkif_x86_32_request blkif_x86_32_request_t;
-typedef struct blkif_x86_32_response blkif_x86_32_response_t;
 #pragma pack(pop)
 
 /* x86_64 protocol version */
@@ -62,20 +53,14 @@ struct blkif_x86_64_request_discard {
     blkif_sector_t sector_number;    /* start sector idx on disk (r/w only)  */
     uint64_t       nr_sectors;       /* # of contiguous sectors to discard   */
 };
-struct blkif_x86_64_response {
-    uint64_t       __attribute__((__aligned__(8))) id;
-    uint8_t         operation;       /* copied from request */
-    int16_t         status;          /* BLKIF_RSP_???       */
-};
 typedef struct blkif_x86_64_request blkif_x86_64_request_t;
-typedef struct blkif_x86_64_response blkif_x86_64_response_t;
 
 DEFINE_RING_TYPES(blkif_common, struct blkif_common_request,
-                  struct blkif_common_response);
+                  struct blkif_response);
 DEFINE_RING_TYPES(blkif_x86_32, struct blkif_x86_32_request,
-                  struct blkif_x86_32_response);
+                  struct blkif_response QEMU_PACKED);
 DEFINE_RING_TYPES(blkif_x86_64, struct blkif_x86_64_request,
-                  struct blkif_x86_64_response);
+                  struct blkif_response);
 
 union blkif_back_rings {
     blkif_back_ring_t        native;
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -769,31 +769,30 @@ static int blk_send_response_one(struct
     struct XenBlkDev  *blkdev = ioreq->blkdev;
     int               send_notify   = 0;
     int               have_requests = 0;
-    blkif_response_t  resp;
-    void              *dst;
-
-    resp.id        = ioreq->req.id;
-    resp.operation = ioreq->req.operation;
-    resp.status    = ioreq->status;
+    blkif_response_t  *resp;
 
     /* Place on the response ring for the relevant domain. */
     switch (blkdev->protocol) {
     case BLKIF_PROTOCOL_NATIVE:
-        dst = RING_GET_RESPONSE(&blkdev->rings.native, blkdev->rings.native.rsp_prod_pvt);
+        resp = RING_GET_RESPONSE(&blkdev->rings.native,
+                                 blkdev->rings.native.rsp_prod_pvt);
         break;
     case BLKIF_PROTOCOL_X86_32:
-        dst = RING_GET_RESPONSE(&blkdev->rings.x86_32_part,
-                                blkdev->rings.x86_32_part.rsp_prod_pvt);
+        resp = RING_GET_RESPONSE(&blkdev->rings.x86_32_part,
+                                 blkdev->rings.x86_32_part.rsp_prod_pvt);
         break;
     case BLKIF_PROTOCOL_X86_64:
-        dst = RING_GET_RESPONSE(&blkdev->rings.x86_64_part,
-                                blkdev->rings.x86_64_part.rsp_prod_pvt);
+        resp = RING_GET_RESPONSE(&blkdev->rings.x86_64_part,
+                                 blkdev->rings.x86_64_part.rsp_prod_pvt);
         break;
     default:
-        dst = NULL;
         return 0;
     }
-    memcpy(dst, &resp, sizeof(resp));
+
+    resp->id        = ioreq->req.id;
+    resp->operation = ioreq->req.operation;
+    resp->status    = ioreq->status;
+
     blkdev->rings.common.rsp_prod_pvt++;
 
     RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&blkdev->rings.common, send_notify);

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

Re: [Qemu-devel] [PATCH] xen/disk: don't leak stack data via response ring

[Stefano Stabellini]

On Tue, 20 Jun 2017, Jan Beulich wrote:
> Rather than constructing a local structure instance on the stack, fill
> the fields directly on the shared ring, just like other (Linux)
> backends do. Build on the fact that all response structure flavors are
> actually identical (the old code did make this assumption too).
> 
> This is XSA-216.
> 
> Reported by: Anthony Perard <anthony.perard@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Acked-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> v2: Add QEMU_PACKED to fix handling 32-bit guests by 64-bit qemu.
> 
> --- a/hw/block/xen_blkif.h
> +++ b/hw/block/xen_blkif.h
> @@ -14,9 +14,6 @@
>  struct blkif_common_request {
>      char dummy;
>  };
> -struct blkif_common_response {
> -    char dummy;
> -};
>  
>  /* i386 protocol version */
>  #pragma pack(push, 4)
> @@ -36,13 +33,7 @@ struct blkif_x86_32_request_discard {
>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w only)  */
>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard   */
>  };
> -struct blkif_x86_32_response {
> -    uint64_t        id;              /* copied from request */
> -    uint8_t         operation;       /* copied from request */
> -    int16_t         status;          /* BLKIF_RSP_???       */
> -};
>  typedef struct blkif_x86_32_request blkif_x86_32_request_t;
> -typedef struct blkif_x86_32_response blkif_x86_32_response_t;
>  #pragma pack(pop)
>  
>  /* x86_64 protocol version */
> @@ -62,20 +53,14 @@ struct blkif_x86_64_request_discard {
>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w only)  */
>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard   */
>  };
> -struct blkif_x86_64_response {
> -    uint64_t       __attribute__((__aligned__(8))) id;
> -    uint8_t         operation;       /* copied from request */
> -    int16_t         status;          /* BLKIF_RSP_???       */
> -};
>
>  typedef struct blkif_x86_64_request blkif_x86_64_request_t;
> -typedef struct blkif_x86_64_response blkif_x86_64_response_t;
>  
>  DEFINE_RING_TYPES(blkif_common, struct blkif_common_request,
> -                  struct blkif_common_response);
> +                  struct blkif_response);
>  DEFINE_RING_TYPES(blkif_x86_32, struct blkif_x86_32_request,
> -                  struct blkif_x86_32_response);
> +                  struct blkif_response QEMU_PACKED);

In my test, the previous sizes and alignments of the response structs
were (on both x86_32 and x86_64):

sizeof(blkif_x86_32_response)=12   sizeof(blkif_x86_64_response)=16
align(blkif_x86_32_response)=4     align(blkif_x86_64_response)=8

While with these changes are now, when compiled on x86_64:
sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=16
align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=8

when compiled on x86_32:
sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=12
align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=4

Did I do my tests wrong?

QEMU_PACKED is not the same as #pragma pack(push, 4). In fact, it is the
same as #pragma pack(push, 1), causing the struct to be densely packed,
leaving no padding whatsever.

In addition, without __attribute__((__aligned__(8))),
blkif_x86_64_response won't be 8 bytes aligned when built on x86_32.

Am I missing something?


>  DEFINE_RING_TYPES(blkif_x86_64, struct blkif_x86_64_request,
> -                  struct blkif_x86_64_response);
> +                  struct blkif_response);
>  
>  union blkif_back_rings {
>      blkif_back_ring_t        native;
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -769,31 +769,30 @@ static int blk_send_response_one(struct
>      struct XenBlkDev  *blkdev = ioreq->blkdev;
>      int               send_notify   = 0;
>      int               have_requests = 0;
> -    blkif_response_t  resp;
> -    void              *dst;
> -
> -    resp.id        = ioreq->req.id;
> -    resp.operation = ioreq->req.operation;
> -    resp.status    = ioreq->status;
> +    blkif_response_t  *resp;
>  
>      /* Place on the response ring for the relevant domain. */
>      switch (blkdev->protocol) {
>      case BLKIF_PROTOCOL_NATIVE:
> -        dst = RING_GET_RESPONSE(&blkdev->rings.native, blkdev->rings.native.rsp_prod_pvt);
> +        resp = RING_GET_RESPONSE(&blkdev->rings.native,
> +                                 blkdev->rings.native.rsp_prod_pvt);
>          break;
>      case BLKIF_PROTOCOL_X86_32:
> -        dst = RING_GET_RESPONSE(&blkdev->rings.x86_32_part,
> -                                blkdev->rings.x86_32_part.rsp_prod_pvt);
> +        resp = RING_GET_RESPONSE(&blkdev->rings.x86_32_part,
> +                                 blkdev->rings.x86_32_part.rsp_prod_pvt);
>          break;
>      case BLKIF_PROTOCOL_X86_64:
> -        dst = RING_GET_RESPONSE(&blkdev->rings.x86_64_part,
> -                                blkdev->rings.x86_64_part.rsp_prod_pvt);
> +        resp = RING_GET_RESPONSE(&blkdev->rings.x86_64_part,
> +                                 blkdev->rings.x86_64_part.rsp_prod_pvt);
>          break;
>      default:
> -        dst = NULL;
>          return 0;
>      }
> -    memcpy(dst, &resp, sizeof(resp));
> +
> +    resp->id        = ioreq->req.id;
> +    resp->operation = ioreq->req.operation;
> +    resp->status    = ioreq->status;
> +
>      blkdev->rings.common.rsp_prod_pvt++;
>  
>      RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&blkdev->rings.common, send_notify);
> 
> 
> 

Re: [PATCH] xen/disk: don't leak stack data via response ring

[Stefano Stabellini]

On Tue, 20 Jun 2017, Jan Beulich wrote:
> Rather than constructing a local structure instance on the stack, fill
> the fields directly on the shared ring, just like other (Linux)
> backends do. Build on the fact that all response structure flavors are
> actually identical (the old code did make this assumption too).
> 
> This is XSA-216.
> 
> Reported by: Anthony Perard <anthony.perard@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Acked-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> v2: Add QEMU_PACKED to fix handling 32-bit guests by 64-bit qemu.
> 
> --- a/hw/block/xen_blkif.h
> +++ b/hw/block/xen_blkif.h
> @@ -14,9 +14,6 @@
>  struct blkif_common_request {
>      char dummy;
>  };
> -struct blkif_common_response {
> -    char dummy;
> -};
>  
>  /* i386 protocol version */
>  #pragma pack(push, 4)
> @@ -36,13 +33,7 @@ struct blkif_x86_32_request_discard {
>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w only)  */
>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard   */
>  };
> -struct blkif_x86_32_response {
> -    uint64_t        id;              /* copied from request */
> -    uint8_t         operation;       /* copied from request */
> -    int16_t         status;          /* BLKIF_RSP_???       */
> -};
>  typedef struct blkif_x86_32_request blkif_x86_32_request_t;
> -typedef struct blkif_x86_32_response blkif_x86_32_response_t;
>  #pragma pack(pop)
>  
>  /* x86_64 protocol version */
> @@ -62,20 +53,14 @@ struct blkif_x86_64_request_discard {
>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w only)  */
>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard   */
>  };
> -struct blkif_x86_64_response {
> -    uint64_t       __attribute__((__aligned__(8))) id;
> -    uint8_t         operation;       /* copied from request */
> -    int16_t         status;          /* BLKIF_RSP_???       */
> -};
>
>  typedef struct blkif_x86_64_request blkif_x86_64_request_t;
> -typedef struct blkif_x86_64_response blkif_x86_64_response_t;
>  
>  DEFINE_RING_TYPES(blkif_common, struct blkif_common_request,
> -                  struct blkif_common_response);
> +                  struct blkif_response);
>  DEFINE_RING_TYPES(blkif_x86_32, struct blkif_x86_32_request,
> -                  struct blkif_x86_32_response);
> +                  struct blkif_response QEMU_PACKED);

In my test, the previous sizes and alignments of the response structs
were (on both x86_32 and x86_64):

sizeof(blkif_x86_32_response)=12   sizeof(blkif_x86_64_response)=16
align(blkif_x86_32_response)=4     align(blkif_x86_64_response)=8

While with these changes are now, when compiled on x86_64:
sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=16
align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=8

when compiled on x86_32:
sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=12
align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=4

Did I do my tests wrong?

QEMU_PACKED is not the same as #pragma pack(push, 4). In fact, it is the
same as #pragma pack(push, 1), causing the struct to be densely packed,
leaving no padding whatsever.

In addition, without __attribute__((__aligned__(8))),
blkif_x86_64_response won't be 8 bytes aligned when built on x86_32.

Am I missing something?


>  DEFINE_RING_TYPES(blkif_x86_64, struct blkif_x86_64_request,
> -                  struct blkif_x86_64_response);
> +                  struct blkif_response);
>  
>  union blkif_back_rings {
>      blkif_back_ring_t        native;
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -769,31 +769,30 @@ static int blk_send_response_one(struct
>      struct XenBlkDev  *blkdev = ioreq->blkdev;
>      int               send_notify   = 0;
>      int               have_requests = 0;
> -    blkif_response_t  resp;
> -    void              *dst;
> -
> -    resp.id        = ioreq->req.id;
> -    resp.operation = ioreq->req.operation;
> -    resp.status    = ioreq->status;
> +    blkif_response_t  *resp;
>  
>      /* Place on the response ring for the relevant domain. */
>      switch (blkdev->protocol) {
>      case BLKIF_PROTOCOL_NATIVE:
> -        dst = RING_GET_RESPONSE(&blkdev->rings.native, blkdev->rings.native.rsp_prod_pvt);
> +        resp = RING_GET_RESPONSE(&blkdev->rings.native,
> +                                 blkdev->rings.native.rsp_prod_pvt);
>          break;
>      case BLKIF_PROTOCOL_X86_32:
> -        dst = RING_GET_RESPONSE(&blkdev->rings.x86_32_part,
> -                                blkdev->rings.x86_32_part.rsp_prod_pvt);
> +        resp = RING_GET_RESPONSE(&blkdev->rings.x86_32_part,
> +                                 blkdev->rings.x86_32_part.rsp_prod_pvt);
>          break;
>      case BLKIF_PROTOCOL_X86_64:
> -        dst = RING_GET_RESPONSE(&blkdev->rings.x86_64_part,
> -                                blkdev->rings.x86_64_part.rsp_prod_pvt);
> +        resp = RING_GET_RESPONSE(&blkdev->rings.x86_64_part,
> +                                 blkdev->rings.x86_64_part.rsp_prod_pvt);
>          break;
>      default:
> -        dst = NULL;
>          return 0;
>      }
> -    memcpy(dst, &resp, sizeof(resp));
> +
> +    resp->id        = ioreq->req.id;
> +    resp->operation = ioreq->req.operation;
> +    resp->status    = ioreq->status;
> +
>      blkdev->rings.common.rsp_prod_pvt++;
>  
>      RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&blkdev->rings.common, send_notify);
> 
> 
> 

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

Re: [Qemu-devel] [PATCH] xen/disk: don't leak stack data via response ring

[Jan Beulich]

>>> On 20.06.17 at 23:48, <sstabellini@kernel.org> wrote:
> On Tue, 20 Jun 2017, Jan Beulich wrote:
>> @@ -36,13 +33,7 @@ struct blkif_x86_32_request_discard {
>>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w only)  */
>>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard   */
>>  };
>> -struct blkif_x86_32_response {
>> -    uint64_t        id;              /* copied from request */
>> -    uint8_t         operation;       /* copied from request */
>> -    int16_t         status;          /* BLKIF_RSP_???       */
>> -};
>>  typedef struct blkif_x86_32_request blkif_x86_32_request_t;
>> -typedef struct blkif_x86_32_response blkif_x86_32_response_t;
>>  #pragma pack(pop)
>>  
>>  /* x86_64 protocol version */
>> @@ -62,20 +53,14 @@ struct blkif_x86_64_request_discard {
>>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w only)  */
>>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard   */
>>  };
>> -struct blkif_x86_64_response {
>> -    uint64_t       __attribute__((__aligned__(8))) id;
>> -    uint8_t         operation;       /* copied from request */
>> -    int16_t         status;          /* BLKIF_RSP_???       */
>> -};
>>
>>  typedef struct blkif_x86_64_request blkif_x86_64_request_t;
>> -typedef struct blkif_x86_64_response blkif_x86_64_response_t;
>>  
>>  DEFINE_RING_TYPES(blkif_common, struct blkif_common_request,
>> -                  struct blkif_common_response);
>> +                  struct blkif_response);
>>  DEFINE_RING_TYPES(blkif_x86_32, struct blkif_x86_32_request,
>> -                  struct blkif_x86_32_response);
>> +                  struct blkif_response QEMU_PACKED);
> 
> In my test, the previous sizes and alignments of the response structs
> were (on both x86_32 and x86_64):
> 
> sizeof(blkif_x86_32_response)=12   sizeof(blkif_x86_64_response)=16
> align(blkif_x86_32_response)=4     align(blkif_x86_64_response)=8
> 
> While with these changes are now, when compiled on x86_64:
> sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=16
> align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=8
> 
> when compiled on x86_32:
> sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=12
> align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=4
> 
> Did I do my tests wrong?
> 
> QEMU_PACKED is not the same as #pragma pack(push, 4). In fact, it is the
> same as #pragma pack(push, 1), causing the struct to be densely packed,
> leaving no padding whatsever.
> 
> In addition, without __attribute__((__aligned__(8))),
> blkif_x86_64_response won't be 8 bytes aligned when built on x86_32.
> 
> Am I missing something?

Well, you're mixing attribute application upon structure
declaration with attribute application upon structure use. It's
the latter here, and hence the attribute doesn't affect
structure layout at all. All it does is avoid the _containing_
32-bit union to become 8-byte aligned (and tail padding to be
inserted).

Jan

Re: [PATCH] xen/disk: don't leak stack data via response ring

[Jan Beulich]

>>> On 20.06.17 at 23:48, <sstabellini@kernel.org> wrote:
> On Tue, 20 Jun 2017, Jan Beulich wrote:
>> @@ -36,13 +33,7 @@ struct blkif_x86_32_request_discard {
>>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w only)  */
>>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard   */
>>  };
>> -struct blkif_x86_32_response {
>> -    uint64_t        id;              /* copied from request */
>> -    uint8_t         operation;       /* copied from request */
>> -    int16_t         status;          /* BLKIF_RSP_???       */
>> -};
>>  typedef struct blkif_x86_32_request blkif_x86_32_request_t;
>> -typedef struct blkif_x86_32_response blkif_x86_32_response_t;
>>  #pragma pack(pop)
>>  
>>  /* x86_64 protocol version */
>> @@ -62,20 +53,14 @@ struct blkif_x86_64_request_discard {
>>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w only)  */
>>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard   */
>>  };
>> -struct blkif_x86_64_response {
>> -    uint64_t       __attribute__((__aligned__(8))) id;
>> -    uint8_t         operation;       /* copied from request */
>> -    int16_t         status;          /* BLKIF_RSP_???       */
>> -};
>>
>>  typedef struct blkif_x86_64_request blkif_x86_64_request_t;
>> -typedef struct blkif_x86_64_response blkif_x86_64_response_t;
>>  
>>  DEFINE_RING_TYPES(blkif_common, struct blkif_common_request,
>> -                  struct blkif_common_response);
>> +                  struct blkif_response);
>>  DEFINE_RING_TYPES(blkif_x86_32, struct blkif_x86_32_request,
>> -                  struct blkif_x86_32_response);
>> +                  struct blkif_response QEMU_PACKED);
> 
> In my test, the previous sizes and alignments of the response structs
> were (on both x86_32 and x86_64):
> 
> sizeof(blkif_x86_32_response)=12   sizeof(blkif_x86_64_response)=16
> align(blkif_x86_32_response)=4     align(blkif_x86_64_response)=8
> 
> While with these changes are now, when compiled on x86_64:
> sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=16
> align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=8
> 
> when compiled on x86_32:
> sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=12
> align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=4
> 
> Did I do my tests wrong?
> 
> QEMU_PACKED is not the same as #pragma pack(push, 4). In fact, it is the
> same as #pragma pack(push, 1), causing the struct to be densely packed,
> leaving no padding whatsever.
> 
> In addition, without __attribute__((__aligned__(8))),
> blkif_x86_64_response won't be 8 bytes aligned when built on x86_32.
> 
> Am I missing something?

Well, you're mixing attribute application upon structure
declaration with attribute application upon structure use. It's
the latter here, and hence the attribute doesn't affect
structure layout at all. All it does is avoid the _containing_
32-bit union to become 8-byte aligned (and tail padding to be
inserted).

Jan


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

Re: [Qemu-devel] [PATCH] xen/disk: don't leak stack data via response ring

[Stefano Stabellini]

On Wed, 21 Jun 2017, Jan Beulich wrote:
> >>> On 20.06.17 at 23:48, <sstabellini@kernel.org> wrote:
> > On Tue, 20 Jun 2017, Jan Beulich wrote:
> >> @@ -36,13 +33,7 @@ struct blkif_x86_32_request_discard {
> >>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w only)  */
> >>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard   */
> >>  };
> >> -struct blkif_x86_32_response {
> >> -    uint64_t        id;              /* copied from request */
> >> -    uint8_t         operation;       /* copied from request */
> >> -    int16_t         status;          /* BLKIF_RSP_???       */
> >> -};
> >>  typedef struct blkif_x86_32_request blkif_x86_32_request_t;
> >> -typedef struct blkif_x86_32_response blkif_x86_32_response_t;
> >>  #pragma pack(pop)
> >>  
> >>  /* x86_64 protocol version */
> >> @@ -62,20 +53,14 @@ struct blkif_x86_64_request_discard {
> >>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w only)  */
> >>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard   */
> >>  };
> >> -struct blkif_x86_64_response {
> >> -    uint64_t       __attribute__((__aligned__(8))) id;
> >> -    uint8_t         operation;       /* copied from request */
> >> -    int16_t         status;          /* BLKIF_RSP_???       */
> >> -};
> >>
> >>  typedef struct blkif_x86_64_request blkif_x86_64_request_t;
> >> -typedef struct blkif_x86_64_response blkif_x86_64_response_t;
> >>  
> >>  DEFINE_RING_TYPES(blkif_common, struct blkif_common_request,
> >> -                  struct blkif_common_response);
> >> +                  struct blkif_response);
> >>  DEFINE_RING_TYPES(blkif_x86_32, struct blkif_x86_32_request,
> >> -                  struct blkif_x86_32_response);
> >> +                  struct blkif_response QEMU_PACKED);
> > 
> > In my test, the previous sizes and alignments of the response structs
> > were (on both x86_32 and x86_64):
> > 
> > sizeof(blkif_x86_32_response)=12   sizeof(blkif_x86_64_response)=16
> > align(blkif_x86_32_response)=4     align(blkif_x86_64_response)=8
> > 
> > While with these changes are now, when compiled on x86_64:
> > sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=16
> > align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=8
> > 
> > when compiled on x86_32:
> > sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=12
> > align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=4
> > 
> > Did I do my tests wrong?
> > 
> > QEMU_PACKED is not the same as #pragma pack(push, 4). In fact, it is the
> > same as #pragma pack(push, 1), causing the struct to be densely packed,
> > leaving no padding whatsever.
> > 
> > In addition, without __attribute__((__aligned__(8))),
> > blkif_x86_64_response won't be 8 bytes aligned when built on x86_32.
> > 
> > Am I missing something?
> 
> Well, you're mixing attribute application upon structure
> declaration with attribute application upon structure use. It's
> the latter here, and hence the attribute doesn't affect
> structure layout at all. All it does is avoid the _containing_
> 32-bit union to become 8-byte aligned (and tail padding to be
> inserted).

Thanks for the explanation. I admit it's the first time I see the
aligned attribute being used at structure usage only. I think it's the
first time QEMU_PACKED is used this way in QEMU too.

Anyway, even taking that into account, things are still not completely
right: the alignment of struct blkif_x86_32_response QEMU_PACKED is 4
bytes as you wrote, but the size of struct blkif_x86_32_response is
still 16 bytes instead of 12 bytes in my test. I suspect it worked for
you because the other member of the union (blkif_x86_32_request) is
larger than that. However, I think is not a good idea to rely on this
implementation detail. The implementation of DEFINE_RING_TYPES should be
opaque from our point of view. We shouldn't have to know that there is a
union there.

Moreover, the other problem is still unaddressed: the size and alignment
of blkif_x86_64_response when built on x86_32 are 12 and 4 instead of 16
and 8 bytes. Is that working also because it's relying on the other
member of the union to enforce the right alignment and bigger size? I
think it's a bad idea to rely on that, especially given that this
obscure but important detail is not even mentioned in the commit message.

Re: [PATCH] xen/disk: don't leak stack data via response ring

[Stefano Stabellini]

On Wed, 21 Jun 2017, Jan Beulich wrote:
> >>> On 20.06.17 at 23:48, <sstabellini@kernel.org> wrote:
> > On Tue, 20 Jun 2017, Jan Beulich wrote:
> >> @@ -36,13 +33,7 @@ struct blkif_x86_32_request_discard {
> >>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w only)  */
> >>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard   */
> >>  };
> >> -struct blkif_x86_32_response {
> >> -    uint64_t        id;              /* copied from request */
> >> -    uint8_t         operation;       /* copied from request */
> >> -    int16_t         status;          /* BLKIF_RSP_???       */
> >> -};
> >>  typedef struct blkif_x86_32_request blkif_x86_32_request_t;
> >> -typedef struct blkif_x86_32_response blkif_x86_32_response_t;
> >>  #pragma pack(pop)
> >>  
> >>  /* x86_64 protocol version */
> >> @@ -62,20 +53,14 @@ struct blkif_x86_64_request_discard {
> >>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w only)  */
> >>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard   */
> >>  };
> >> -struct blkif_x86_64_response {
> >> -    uint64_t       __attribute__((__aligned__(8))) id;
> >> -    uint8_t         operation;       /* copied from request */
> >> -    int16_t         status;          /* BLKIF_RSP_???       */
> >> -};
> >>
> >>  typedef struct blkif_x86_64_request blkif_x86_64_request_t;
> >> -typedef struct blkif_x86_64_response blkif_x86_64_response_t;
> >>  
> >>  DEFINE_RING_TYPES(blkif_common, struct blkif_common_request,
> >> -                  struct blkif_common_response);
> >> +                  struct blkif_response);
> >>  DEFINE_RING_TYPES(blkif_x86_32, struct blkif_x86_32_request,
> >> -                  struct blkif_x86_32_response);
> >> +                  struct blkif_response QEMU_PACKED);
> > 
> > In my test, the previous sizes and alignments of the response structs
> > were (on both x86_32 and x86_64):
> > 
> > sizeof(blkif_x86_32_response)=12   sizeof(blkif_x86_64_response)=16
> > align(blkif_x86_32_response)=4     align(blkif_x86_64_response)=8
> > 
> > While with these changes are now, when compiled on x86_64:
> > sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=16
> > align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=8
> > 
> > when compiled on x86_32:
> > sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=12
> > align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=4
> > 
> > Did I do my tests wrong?
> > 
> > QEMU_PACKED is not the same as #pragma pack(push, 4). In fact, it is the
> > same as #pragma pack(push, 1), causing the struct to be densely packed,
> > leaving no padding whatsever.
> > 
> > In addition, without __attribute__((__aligned__(8))),
> > blkif_x86_64_response won't be 8 bytes aligned when built on x86_32.
> > 
> > Am I missing something?
> 
> Well, you're mixing attribute application upon structure
> declaration with attribute application upon structure use. It's
> the latter here, and hence the attribute doesn't affect
> structure layout at all. All it does is avoid the _containing_
> 32-bit union to become 8-byte aligned (and tail padding to be
> inserted).

Thanks for the explanation. I admit it's the first time I see the
aligned attribute being used at structure usage only. I think it's the
first time QEMU_PACKED is used this way in QEMU too.

Anyway, even taking that into account, things are still not completely
right: the alignment of struct blkif_x86_32_response QEMU_PACKED is 4
bytes as you wrote, but the size of struct blkif_x86_32_response is
still 16 bytes instead of 12 bytes in my test. I suspect it worked for
you because the other member of the union (blkif_x86_32_request) is
larger than that. However, I think is not a good idea to rely on this
implementation detail. The implementation of DEFINE_RING_TYPES should be
opaque from our point of view. We shouldn't have to know that there is a
union there.

Moreover, the other problem is still unaddressed: the size and alignment
of blkif_x86_64_response when built on x86_32 are 12 and 4 instead of 16
and 8 bytes. Is that working also because it's relying on the other
member of the union to enforce the right alignment and bigger size? I
think it's a bad idea to rely on that, especially given that this
obscure but important detail is not even mentioned in the commit message.

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

Re: [Qemu-devel] [PATCH] xen/disk: don't leak stack data via response ring

[Jan Beulich]

>>> On 21.06.17 at 20:46, <sstabellini@kernel.org> wrote:
> On Wed, 21 Jun 2017, Jan Beulich wrote:
>> >>> On 20.06.17 at 23:48, <sstabellini@kernel.org> wrote:
>> > On Tue, 20 Jun 2017, Jan Beulich wrote:
>> >> @@ -36,13 +33,7 @@ struct blkif_x86_32_request_discard {
>> >>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w 
> only)  */
>> >>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard 
>   */
>> >>  };
>> >> -struct blkif_x86_32_response {
>> >> -    uint64_t        id;              /* copied from request */
>> >> -    uint8_t         operation;       /* copied from request */
>> >> -    int16_t         status;          /* BLKIF_RSP_???       */
>> >> -};
>> >>  typedef struct blkif_x86_32_request blkif_x86_32_request_t;
>> >> -typedef struct blkif_x86_32_response blkif_x86_32_response_t;
>> >>  #pragma pack(pop)
>> >>  
>> >>  /* x86_64 protocol version */
>> >> @@ -62,20 +53,14 @@ struct blkif_x86_64_request_discard {
>> >>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w 
> only)  */
>> >>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard 
>   */
>> >>  };
>> >> -struct blkif_x86_64_response {
>> >> -    uint64_t       __attribute__((__aligned__(8))) id;
>> >> -    uint8_t         operation;       /* copied from request */
>> >> -    int16_t         status;          /* BLKIF_RSP_???       */
>> >> -};
>> >>
>> >>  typedef struct blkif_x86_64_request blkif_x86_64_request_t;
>> >> -typedef struct blkif_x86_64_response blkif_x86_64_response_t;
>> >>  
>> >>  DEFINE_RING_TYPES(blkif_common, struct blkif_common_request,
>> >> -                  struct blkif_common_response);
>> >> +                  struct blkif_response);
>> >>  DEFINE_RING_TYPES(blkif_x86_32, struct blkif_x86_32_request,
>> >> -                  struct blkif_x86_32_response);
>> >> +                  struct blkif_response QEMU_PACKED);
>> > 
>> > In my test, the previous sizes and alignments of the response structs
>> > were (on both x86_32 and x86_64):
>> > 
>> > sizeof(blkif_x86_32_response)=12   sizeof(blkif_x86_64_response)=16
>> > align(blkif_x86_32_response)=4     align(blkif_x86_64_response)=8
>> > 
>> > While with these changes are now, when compiled on x86_64:
>> > sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=16
>> > align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=8
>> > 
>> > when compiled on x86_32:
>> > sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=12
>> > align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=4
>> > 
>> > Did I do my tests wrong?
>> > 
>> > QEMU_PACKED is not the same as #pragma pack(push, 4). In fact, it is the
>> > same as #pragma pack(push, 1), causing the struct to be densely packed,
>> > leaving no padding whatsever.
>> > 
>> > In addition, without __attribute__((__aligned__(8))),
>> > blkif_x86_64_response won't be 8 bytes aligned when built on x86_32.
>> > 
>> > Am I missing something?
>> 
>> Well, you're mixing attribute application upon structure
>> declaration with attribute application upon structure use. It's
>> the latter here, and hence the attribute doesn't affect
>> structure layout at all. All it does is avoid the _containing_
>> 32-bit union to become 8-byte aligned (and tail padding to be
>> inserted).
> 
> Thanks for the explanation. I admit it's the first time I see the
> aligned attribute being used at structure usage only. I think it's the
> first time QEMU_PACKED is used this way in QEMU too.
> 
> Anyway, even taking that into account, things are still not completely
> right: the alignment of struct blkif_x86_32_response QEMU_PACKED is 4
> bytes as you wrote, but the size of struct blkif_x86_32_response is
> still 16 bytes instead of 12 bytes in my test. I suspect it worked for
> you because the other member of the union (blkif_x86_32_request) is
> larger than that. However, I think is not a good idea to rely on this
> implementation detail. The implementation of DEFINE_RING_TYPES should be
> opaque from our point of view. We shouldn't have to know that there is a
> union there.

I don't follow - why should we not rely on this? It is a fundamental
aspect of the shared ring model that requests and responses share
space.

> Moreover, the other problem is still unaddressed: the size and alignment
> of blkif_x86_64_response when built on x86_32 are 12 and 4 instead of 16
> and 8 bytes. Is that working also because it's relying on the other
> member of the union to enforce the right alignment and bigger size?

Yes. For these as well as your comments further up - sizeof() and
alignof() are completely uninteresting as long as we don't
instantiate objects of those types _and then use them for
communication_. The patch specifically removes instantiation,
switching to a purely pointer based approach. And that is ...

> I think it's a bad idea to rely on that, especially given that this
> obscure but important detail is not even mentioned in the commit message.

... what the commit message explicitly says (except that of course
the reason for doing so is not the sizeof/alignof aspect, but the
security issue we mean to fix). Everything else is just mechanical
detail of how the goal is being achieved. I don't really mind
extending the commit message (admitting that I'm known to not
write the best ones), but I also don't really see the need.

In the end I'm surprised the qemu side is proving so much more
difficult to get accepted compared to the Linux one.

Jan

Re: [PATCH] xen/disk: don't leak stack data via response ring

[Jan Beulich]

>>> On 21.06.17 at 20:46, <sstabellini@kernel.org> wrote:
> On Wed, 21 Jun 2017, Jan Beulich wrote:
>> >>> On 20.06.17 at 23:48, <sstabellini@kernel.org> wrote:
>> > On Tue, 20 Jun 2017, Jan Beulich wrote:
>> >> @@ -36,13 +33,7 @@ struct blkif_x86_32_request_discard {
>> >>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w 
> only)  */
>> >>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard 
>   */
>> >>  };
>> >> -struct blkif_x86_32_response {
>> >> -    uint64_t        id;              /* copied from request */
>> >> -    uint8_t         operation;       /* copied from request */
>> >> -    int16_t         status;          /* BLKIF_RSP_???       */
>> >> -};
>> >>  typedef struct blkif_x86_32_request blkif_x86_32_request_t;
>> >> -typedef struct blkif_x86_32_response blkif_x86_32_response_t;
>> >>  #pragma pack(pop)
>> >>  
>> >>  /* x86_64 protocol version */
>> >> @@ -62,20 +53,14 @@ struct blkif_x86_64_request_discard {
>> >>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w 
> only)  */
>> >>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard 
>   */
>> >>  };
>> >> -struct blkif_x86_64_response {
>> >> -    uint64_t       __attribute__((__aligned__(8))) id;
>> >> -    uint8_t         operation;       /* copied from request */
>> >> -    int16_t         status;          /* BLKIF_RSP_???       */
>> >> -};
>> >>
>> >>  typedef struct blkif_x86_64_request blkif_x86_64_request_t;
>> >> -typedef struct blkif_x86_64_response blkif_x86_64_response_t;
>> >>  
>> >>  DEFINE_RING_TYPES(blkif_common, struct blkif_common_request,
>> >> -                  struct blkif_common_response);
>> >> +                  struct blkif_response);
>> >>  DEFINE_RING_TYPES(blkif_x86_32, struct blkif_x86_32_request,
>> >> -                  struct blkif_x86_32_response);
>> >> +                  struct blkif_response QEMU_PACKED);
>> > 
>> > In my test, the previous sizes and alignments of the response structs
>> > were (on both x86_32 and x86_64):
>> > 
>> > sizeof(blkif_x86_32_response)=12   sizeof(blkif_x86_64_response)=16
>> > align(blkif_x86_32_response)=4     align(blkif_x86_64_response)=8
>> > 
>> > While with these changes are now, when compiled on x86_64:
>> > sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=16
>> > align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=8
>> > 
>> > when compiled on x86_32:
>> > sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=12
>> > align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=4
>> > 
>> > Did I do my tests wrong?
>> > 
>> > QEMU_PACKED is not the same as #pragma pack(push, 4). In fact, it is the
>> > same as #pragma pack(push, 1), causing the struct to be densely packed,
>> > leaving no padding whatsever.
>> > 
>> > In addition, without __attribute__((__aligned__(8))),
>> > blkif_x86_64_response won't be 8 bytes aligned when built on x86_32.
>> > 
>> > Am I missing something?
>> 
>> Well, you're mixing attribute application upon structure
>> declaration with attribute application upon structure use. It's
>> the latter here, and hence the attribute doesn't affect
>> structure layout at all. All it does is avoid the _containing_
>> 32-bit union to become 8-byte aligned (and tail padding to be
>> inserted).
> 
> Thanks for the explanation. I admit it's the first time I see the
> aligned attribute being used at structure usage only. I think it's the
> first time QEMU_PACKED is used this way in QEMU too.
> 
> Anyway, even taking that into account, things are still not completely
> right: the alignment of struct blkif_x86_32_response QEMU_PACKED is 4
> bytes as you wrote, but the size of struct blkif_x86_32_response is
> still 16 bytes instead of 12 bytes in my test. I suspect it worked for
> you because the other member of the union (blkif_x86_32_request) is
> larger than that. However, I think is not a good idea to rely on this
> implementation detail. The implementation of DEFINE_RING_TYPES should be
> opaque from our point of view. We shouldn't have to know that there is a
> union there.

I don't follow - why should we not rely on this? It is a fundamental
aspect of the shared ring model that requests and responses share
space.

> Moreover, the other problem is still unaddressed: the size and alignment
> of blkif_x86_64_response when built on x86_32 are 12 and 4 instead of 16
> and 8 bytes. Is that working also because it's relying on the other
> member of the union to enforce the right alignment and bigger size?

Yes. For these as well as your comments further up - sizeof() and
alignof() are completely uninteresting as long as we don't
instantiate objects of those types _and then use them for
communication_. The patch specifically removes instantiation,
switching to a purely pointer based approach. And that is ...

> I think it's a bad idea to rely on that, especially given that this
> obscure but important detail is not even mentioned in the commit message.

... what the commit message explicitly says (except that of course
the reason for doing so is not the sizeof/alignof aspect, but the
security issue we mean to fix). Everything else is just mechanical
detail of how the goal is being achieved. I don't really mind
extending the commit message (admitting that I'm known to not
write the best ones), but I also don't really see the need.

In the end I'm surprised the qemu side is proving so much more
difficult to get accepted compared to the Linux one.

Jan

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

Re: [Qemu-devel] [PATCH] xen/disk: don't leak stack data via response ring

[Stefano Stabellini]

On Thu, 22 Jun 2017, Jan Beulich wrote:
> >>> On 21.06.17 at 20:46, <sstabellini@kernel.org> wrote:
> > On Wed, 21 Jun 2017, Jan Beulich wrote:
> >> >>> On 20.06.17 at 23:48, <sstabellini@kernel.org> wrote:
> >> > On Tue, 20 Jun 2017, Jan Beulich wrote:
> >> >> @@ -36,13 +33,7 @@ struct blkif_x86_32_request_discard {
> >> >>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w 
> > only)  */
> >> >>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard 
> >   */
> >> >>  };
> >> >> -struct blkif_x86_32_response {
> >> >> -    uint64_t        id;              /* copied from request */
> >> >> -    uint8_t         operation;       /* copied from request */
> >> >> -    int16_t         status;          /* BLKIF_RSP_???       */
> >> >> -};
> >> >>  typedef struct blkif_x86_32_request blkif_x86_32_request_t;
> >> >> -typedef struct blkif_x86_32_response blkif_x86_32_response_t;
> >> >>  #pragma pack(pop)
> >> >>  
> >> >>  /* x86_64 protocol version */
> >> >> @@ -62,20 +53,14 @@ struct blkif_x86_64_request_discard {
> >> >>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w 
> > only)  */
> >> >>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard 
> >   */
> >> >>  };
> >> >> -struct blkif_x86_64_response {
> >> >> -    uint64_t       __attribute__((__aligned__(8))) id;
> >> >> -    uint8_t         operation;       /* copied from request */
> >> >> -    int16_t         status;          /* BLKIF_RSP_???       */
> >> >> -};
> >> >>
> >> >>  typedef struct blkif_x86_64_request blkif_x86_64_request_t;
> >> >> -typedef struct blkif_x86_64_response blkif_x86_64_response_t;
> >> >>  
> >> >>  DEFINE_RING_TYPES(blkif_common, struct blkif_common_request,
> >> >> -                  struct blkif_common_response);
> >> >> +                  struct blkif_response);
> >> >>  DEFINE_RING_TYPES(blkif_x86_32, struct blkif_x86_32_request,
> >> >> -                  struct blkif_x86_32_response);
> >> >> +                  struct blkif_response QEMU_PACKED);
> >> > 
> >> > In my test, the previous sizes and alignments of the response structs
> >> > were (on both x86_32 and x86_64):
> >> > 
> >> > sizeof(blkif_x86_32_response)=12   sizeof(blkif_x86_64_response)=16
> >> > align(blkif_x86_32_response)=4     align(blkif_x86_64_response)=8
> >> > 
> >> > While with these changes are now, when compiled on x86_64:
> >> > sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=16
> >> > align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=8
> >> > 
> >> > when compiled on x86_32:
> >> > sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=12
> >> > align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=4
> >> > 
> >> > Did I do my tests wrong?
> >> > 
> >> > QEMU_PACKED is not the same as #pragma pack(push, 4). In fact, it is the
> >> > same as #pragma pack(push, 1), causing the struct to be densely packed,
> >> > leaving no padding whatsever.
> >> > 
> >> > In addition, without __attribute__((__aligned__(8))),
> >> > blkif_x86_64_response won't be 8 bytes aligned when built on x86_32.
> >> > 
> >> > Am I missing something?
> >> 
> >> Well, you're mixing attribute application upon structure
> >> declaration with attribute application upon structure use. It's
> >> the latter here, and hence the attribute doesn't affect
> >> structure layout at all. All it does is avoid the _containing_
> >> 32-bit union to become 8-byte aligned (and tail padding to be
> >> inserted).
> > 
> > Thanks for the explanation. I admit it's the first time I see the
> > aligned attribute being used at structure usage only. I think it's the
> > first time QEMU_PACKED is used this way in QEMU too.
> > 
> > Anyway, even taking that into account, things are still not completely
> > right: the alignment of struct blkif_x86_32_response QEMU_PACKED is 4
> > bytes as you wrote, but the size of struct blkif_x86_32_response is
> > still 16 bytes instead of 12 bytes in my test. I suspect it worked for
> > you because the other member of the union (blkif_x86_32_request) is
> > larger than that. However, I think is not a good idea to rely on this
> > implementation detail. The implementation of DEFINE_RING_TYPES should be
> > opaque from our point of view. We shouldn't have to know that there is a
> > union there.
> 
> I don't follow - why should we not rely on this? It is a fundamental
> aspect of the shared ring model that requests and responses share
> space.
> 
> > Moreover, the other problem is still unaddressed: the size and alignment
> > of blkif_x86_64_response when built on x86_32 are 12 and 4 instead of 16
> > and 8 bytes. Is that working also because it's relying on the other
> > member of the union to enforce the right alignment and bigger size?
> 
> Yes. For these as well as your comments further up - sizeof() and
> alignof() are completely uninteresting as long as we don't
> instantiate objects of those types _and then use them for
> communication_. The patch specifically removes instantiation,
> switching to a purely pointer based approach. And that is ...

As long as we don't instantiate objects of those types and we use them
for communication? This is basically a death trap hidden in an innocuous
looking header file.


> In the end I'm surprised the qemu side is proving so much more
> difficult to get accepted compared to the Linux one.

I am happy to write the code and/or the commit message. Would a simple
cast like below work to fix the security issue?


diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 3a22805..9200511 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -769,31 +769,30 @@ static int blk_send_response_one(struct ioreq *ioreq)
     struct XenBlkDev  *blkdev = ioreq->blkdev;
     int               send_notify   = 0;
     int               have_requests = 0;
-    blkif_response_t  resp;
-    void              *dst;
-
-    resp.id        = ioreq->req.id;
-    resp.operation = ioreq->req.operation;
-    resp.status    = ioreq->status;
+    blkif_response_t  *resp;
 
     /* Place on the response ring for the relevant domain. */
     switch (blkdev->protocol) {
     case BLKIF_PROTOCOL_NATIVE:
-        dst = RING_GET_RESPONSE(&blkdev->rings.native, blkdev->rings.native.rsp_prod_pvt);
+        resp = (blkif_response_t *) RING_GET_RESPONSE(&blkdev->rings.native,
+                                 blkdev->rings.native.rsp_prod_pvt);
         break;
     case BLKIF_PROTOCOL_X86_32:
-        dst = RING_GET_RESPONSE(&blkdev->rings.x86_32_part,
-                                blkdev->rings.x86_32_part.rsp_prod_pvt);
+        resp = (blkif_response_t *) RING_GET_RESPONSE(&blkdev->rings.x86_32_part,
+                                 blkdev->rings.x86_32_part.rsp_prod_pvt);
         break;
     case BLKIF_PROTOCOL_X86_64:
-        dst = RING_GET_RESPONSE(&blkdev->rings.x86_64_part,
-                                blkdev->rings.x86_64_part.rsp_prod_pvt);
+        resp = (blkif_response_t *) RING_GET_RESPONSE(&blkdev->rings.x86_64_part,
+                                 blkdev->rings.x86_64_part.rsp_prod_pvt);
         break;
     default:
-        dst = NULL;
         return 0;
     }
-    memcpy(dst, &resp, sizeof(resp));
+
+    resp->id        = ioreq->req.id;
+    resp->operation = ioreq->req.operation;
+    resp->status    = ioreq->status;
+
     blkdev->rings.common.rsp_prod_pvt++;
 
     RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&blkdev->rings.common, send_notify);

Re: [PATCH] xen/disk: don't leak stack data via response ring

[Stefano Stabellini]

On Thu, 22 Jun 2017, Jan Beulich wrote:
> >>> On 21.06.17 at 20:46, <sstabellini@kernel.org> wrote:
> > On Wed, 21 Jun 2017, Jan Beulich wrote:
> >> >>> On 20.06.17 at 23:48, <sstabellini@kernel.org> wrote:
> >> > On Tue, 20 Jun 2017, Jan Beulich wrote:
> >> >> @@ -36,13 +33,7 @@ struct blkif_x86_32_request_discard {
> >> >>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w 
> > only)  */
> >> >>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard 
> >   */
> >> >>  };
> >> >> -struct blkif_x86_32_response {
> >> >> -    uint64_t        id;              /* copied from request */
> >> >> -    uint8_t         operation;       /* copied from request */
> >> >> -    int16_t         status;          /* BLKIF_RSP_???       */
> >> >> -};
> >> >>  typedef struct blkif_x86_32_request blkif_x86_32_request_t;
> >> >> -typedef struct blkif_x86_32_response blkif_x86_32_response_t;
> >> >>  #pragma pack(pop)
> >> >>  
> >> >>  /* x86_64 protocol version */
> >> >> @@ -62,20 +53,14 @@ struct blkif_x86_64_request_discard {
> >> >>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w 
> > only)  */
> >> >>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard 
> >   */
> >> >>  };
> >> >> -struct blkif_x86_64_response {
> >> >> -    uint64_t       __attribute__((__aligned__(8))) id;
> >> >> -    uint8_t         operation;       /* copied from request */
> >> >> -    int16_t         status;          /* BLKIF_RSP_???       */
> >> >> -};
> >> >>
> >> >>  typedef struct blkif_x86_64_request blkif_x86_64_request_t;
> >> >> -typedef struct blkif_x86_64_response blkif_x86_64_response_t;
> >> >>  
> >> >>  DEFINE_RING_TYPES(blkif_common, struct blkif_common_request,
> >> >> -                  struct blkif_common_response);
> >> >> +                  struct blkif_response);
> >> >>  DEFINE_RING_TYPES(blkif_x86_32, struct blkif_x86_32_request,
> >> >> -                  struct blkif_x86_32_response);
> >> >> +                  struct blkif_response QEMU_PACKED);
> >> > 
> >> > In my test, the previous sizes and alignments of the response structs
> >> > were (on both x86_32 and x86_64):
> >> > 
> >> > sizeof(blkif_x86_32_response)=12   sizeof(blkif_x86_64_response)=16
> >> > align(blkif_x86_32_response)=4     align(blkif_x86_64_response)=8
> >> > 
> >> > While with these changes are now, when compiled on x86_64:
> >> > sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=16
> >> > align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=8
> >> > 
> >> > when compiled on x86_32:
> >> > sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=12
> >> > align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=4
> >> > 
> >> > Did I do my tests wrong?
> >> > 
> >> > QEMU_PACKED is not the same as #pragma pack(push, 4). In fact, it is the
> >> > same as #pragma pack(push, 1), causing the struct to be densely packed,
> >> > leaving no padding whatsever.
> >> > 
> >> > In addition, without __attribute__((__aligned__(8))),
> >> > blkif_x86_64_response won't be 8 bytes aligned when built on x86_32.
> >> > 
> >> > Am I missing something?
> >> 
> >> Well, you're mixing attribute application upon structure
> >> declaration with attribute application upon structure use. It's
> >> the latter here, and hence the attribute doesn't affect
> >> structure layout at all. All it does is avoid the _containing_
> >> 32-bit union to become 8-byte aligned (and tail padding to be
> >> inserted).
> > 
> > Thanks for the explanation. I admit it's the first time I see the
> > aligned attribute being used at structure usage only. I think it's the
> > first time QEMU_PACKED is used this way in QEMU too.
> > 
> > Anyway, even taking that into account, things are still not completely
> > right: the alignment of struct blkif_x86_32_response QEMU_PACKED is 4
> > bytes as you wrote, but the size of struct blkif_x86_32_response is
> > still 16 bytes instead of 12 bytes in my test. I suspect it worked for
> > you because the other member of the union (blkif_x86_32_request) is
> > larger than that. However, I think is not a good idea to rely on this
> > implementation detail. The implementation of DEFINE_RING_TYPES should be
> > opaque from our point of view. We shouldn't have to know that there is a
> > union there.
> 
> I don't follow - why should we not rely on this? It is a fundamental
> aspect of the shared ring model that requests and responses share
> space.
> 
> > Moreover, the other problem is still unaddressed: the size and alignment
> > of blkif_x86_64_response when built on x86_32 are 12 and 4 instead of 16
> > and 8 bytes. Is that working also because it's relying on the other
> > member of the union to enforce the right alignment and bigger size?
> 
> Yes. For these as well as your comments further up - sizeof() and
> alignof() are completely uninteresting as long as we don't
> instantiate objects of those types _and then use them for
> communication_. The patch specifically removes instantiation,
> switching to a purely pointer based approach. And that is ...

As long as we don't instantiate objects of those types and we use them
for communication? This is basically a death trap hidden in an innocuous
looking header file.


> In the end I'm surprised the qemu side is proving so much more
> difficult to get accepted compared to the Linux one.

I am happy to write the code and/or the commit message. Would a simple
cast like below work to fix the security issue?


diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 3a22805..9200511 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -769,31 +769,30 @@ static int blk_send_response_one(struct ioreq *ioreq)
     struct XenBlkDev  *blkdev = ioreq->blkdev;
     int               send_notify   = 0;
     int               have_requests = 0;
-    blkif_response_t  resp;
-    void              *dst;
-
-    resp.id        = ioreq->req.id;
-    resp.operation = ioreq->req.operation;
-    resp.status    = ioreq->status;
+    blkif_response_t  *resp;
 
     /* Place on the response ring for the relevant domain. */
     switch (blkdev->protocol) {
     case BLKIF_PROTOCOL_NATIVE:
-        dst = RING_GET_RESPONSE(&blkdev->rings.native, blkdev->rings.native.rsp_prod_pvt);
+        resp = (blkif_response_t *) RING_GET_RESPONSE(&blkdev->rings.native,
+                                 blkdev->rings.native.rsp_prod_pvt);
         break;
     case BLKIF_PROTOCOL_X86_32:
-        dst = RING_GET_RESPONSE(&blkdev->rings.x86_32_part,
-                                blkdev->rings.x86_32_part.rsp_prod_pvt);
+        resp = (blkif_response_t *) RING_GET_RESPONSE(&blkdev->rings.x86_32_part,
+                                 blkdev->rings.x86_32_part.rsp_prod_pvt);
         break;
     case BLKIF_PROTOCOL_X86_64:
-        dst = RING_GET_RESPONSE(&blkdev->rings.x86_64_part,
-                                blkdev->rings.x86_64_part.rsp_prod_pvt);
+        resp = (blkif_response_t *) RING_GET_RESPONSE(&blkdev->rings.x86_64_part,
+                                 blkdev->rings.x86_64_part.rsp_prod_pvt);
         break;
     default:
-        dst = NULL;
         return 0;
     }
-    memcpy(dst, &resp, sizeof(resp));
+
+    resp->id        = ioreq->req.id;
+    resp->operation = ioreq->req.operation;
+    resp->status    = ioreq->status;
+
     blkdev->rings.common.rsp_prod_pvt++;
 
     RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&blkdev->rings.common, send_notify);

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

Re: [Qemu-devel] [PATCH] xen/disk: don't leak stack data via response ring

[Jan Beulich]

>>> On 22.06.17 at 20:52, <sstabellini@kernel.org> wrote:
> On Thu, 22 Jun 2017, Jan Beulich wrote:
>> >>> On 21.06.17 at 20:46, <sstabellini@kernel.org> wrote:
>> > On Wed, 21 Jun 2017, Jan Beulich wrote:
>> >> >>> On 20.06.17 at 23:48, <sstabellini@kernel.org> wrote:
>> >> > On Tue, 20 Jun 2017, Jan Beulich wrote:
>> >> >> @@ -36,13 +33,7 @@ struct blkif_x86_32_request_discard {
>> >> >>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w 
>> > only)  */
>> >> >>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard 
>> >   */
>> >> >>  };
>> >> >> -struct blkif_x86_32_response {
>> >> >> -    uint64_t        id;              /* copied from request */
>> >> >> -    uint8_t         operation;       /* copied from request */
>> >> >> -    int16_t         status;          /* BLKIF_RSP_???       */
>> >> >> -};
>> >> >>  typedef struct blkif_x86_32_request blkif_x86_32_request_t;
>> >> >> -typedef struct blkif_x86_32_response blkif_x86_32_response_t;
>> >> >>  #pragma pack(pop)
>> >> >>  
>> >> >>  /* x86_64 protocol version */
>> >> >> @@ -62,20 +53,14 @@ struct blkif_x86_64_request_discard {
>> >> >>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w 
>> > only)  */
>> >> >>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard 
>> >   */
>> >> >>  };
>> >> >> -struct blkif_x86_64_response {
>> >> >> -    uint64_t       __attribute__((__aligned__(8))) id;
>> >> >> -    uint8_t         operation;       /* copied from request */
>> >> >> -    int16_t         status;          /* BLKIF_RSP_???       */
>> >> >> -};
>> >> >>
>> >> >>  typedef struct blkif_x86_64_request blkif_x86_64_request_t;
>> >> >> -typedef struct blkif_x86_64_response blkif_x86_64_response_t;
>> >> >>  
>> >> >>  DEFINE_RING_TYPES(blkif_common, struct blkif_common_request,
>> >> >> -                  struct blkif_common_response);
>> >> >> +                  struct blkif_response);
>> >> >>  DEFINE_RING_TYPES(blkif_x86_32, struct blkif_x86_32_request,
>> >> >> -                  struct blkif_x86_32_response);
>> >> >> +                  struct blkif_response QEMU_PACKED);
>> >> > 
>> >> > In my test, the previous sizes and alignments of the response structs
>> >> > were (on both x86_32 and x86_64):
>> >> > 
>> >> > sizeof(blkif_x86_32_response)=12   sizeof(blkif_x86_64_response)=16
>> >> > align(blkif_x86_32_response)=4     align(blkif_x86_64_response)=8
>> >> > 
>> >> > While with these changes are now, when compiled on x86_64:
>> >> > sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=16
>> >> > align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=8
>> >> > 
>> >> > when compiled on x86_32:
>> >> > sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=12
>> >> > align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=4
>> >> > 
>> >> > Did I do my tests wrong?
>> >> > 
>> >> > QEMU_PACKED is not the same as #pragma pack(push, 4). In fact, it is the
>> >> > same as #pragma pack(push, 1), causing the struct to be densely packed,
>> >> > leaving no padding whatsever.
>> >> > 
>> >> > In addition, without __attribute__((__aligned__(8))),
>> >> > blkif_x86_64_response won't be 8 bytes aligned when built on x86_32.
>> >> > 
>> >> > Am I missing something?
>> >> 
>> >> Well, you're mixing attribute application upon structure
>> >> declaration with attribute application upon structure use. It's
>> >> the latter here, and hence the attribute doesn't affect
>> >> structure layout at all. All it does is avoid the _containing_
>> >> 32-bit union to become 8-byte aligned (and tail padding to be
>> >> inserted).
>> > 
>> > Thanks for the explanation. I admit it's the first time I see the
>> > aligned attribute being used at structure usage only. I think it's the
>> > first time QEMU_PACKED is used this way in QEMU too.
>> > 
>> > Anyway, even taking that into account, things are still not completely
>> > right: the alignment of struct blkif_x86_32_response QEMU_PACKED is 4
>> > bytes as you wrote, but the size of struct blkif_x86_32_response is
>> > still 16 bytes instead of 12 bytes in my test. I suspect it worked for
>> > you because the other member of the union (blkif_x86_32_request) is
>> > larger than that. However, I think is not a good idea to rely on this
>> > implementation detail. The implementation of DEFINE_RING_TYPES should be
>> > opaque from our point of view. We shouldn't have to know that there is a
>> > union there.
>> 
>> I don't follow - why should we not rely on this? It is a fundamental
>> aspect of the shared ring model that requests and responses share
>> space.
>> 
>> > Moreover, the other problem is still unaddressed: the size and alignment
>> > of blkif_x86_64_response when built on x86_32 are 12 and 4 instead of 16
>> > and 8 bytes. Is that working also because it's relying on the other
>> > member of the union to enforce the right alignment and bigger size?
>> 
>> Yes. For these as well as your comments further up - sizeof() and
>> alignof() are completely uninteresting as long as we don't
>> instantiate objects of those types _and then use them for
>> communication_. The patch specifically removes instantiation,
>> switching to a purely pointer based approach. And that is ...
> 
> As long as we don't instantiate objects of those types and we use them
> for communication? This is basically a death trap hidden in an innocuous
> looking header file.

I'm sorry, but - what? Neither frontend nor backend are supposed
to have local instantiations of the structure _and_ use those for
communication. Doing just one of the two is fine, with the caveat
of risking to re-introduce the issue being fixed here if done so in a
backend. Of course the same would apply to request structures
used by a frontend if it cares to not leak information to the
backend.

To outright prevent such issues to occur, it would be a good thing
if the structures could be marked such that the compiler would
refuse to create instantiations (i.e. only pointers to the structure
would be allowed). But since we'd have to arrange for that at
structure declaration point, and since standard C89 doesn't have
ways to arrange for that (and I'd have to see whether there's a
gcc or C99 extension allowing to do so, as I can't think of one
right away), we simply can't do so.

>> In the end I'm surprised the qemu side is proving so much more
>> difficult to get accepted compared to the Linux one.
> 
> I am happy to write the code and/or the commit message. Would a simple
> cast like below work to fix the security issue?

I suppose so, but you do realize that this is _exactly_ what
my patch does, except you use dangerous casts while I play
type-safe?

Jan

Re: [PATCH] xen/disk: don't leak stack data via response ring

[Jan Beulich]

>>> On 22.06.17 at 20:52, <sstabellini@kernel.org> wrote:
> On Thu, 22 Jun 2017, Jan Beulich wrote:
>> >>> On 21.06.17 at 20:46, <sstabellini@kernel.org> wrote:
>> > On Wed, 21 Jun 2017, Jan Beulich wrote:
>> >> >>> On 20.06.17 at 23:48, <sstabellini@kernel.org> wrote:
>> >> > On Tue, 20 Jun 2017, Jan Beulich wrote:
>> >> >> @@ -36,13 +33,7 @@ struct blkif_x86_32_request_discard {
>> >> >>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w 
>> > only)  */
>> >> >>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard 
>> >   */
>> >> >>  };
>> >> >> -struct blkif_x86_32_response {
>> >> >> -    uint64_t        id;              /* copied from request */
>> >> >> -    uint8_t         operation;       /* copied from request */
>> >> >> -    int16_t         status;          /* BLKIF_RSP_???       */
>> >> >> -};
>> >> >>  typedef struct blkif_x86_32_request blkif_x86_32_request_t;
>> >> >> -typedef struct blkif_x86_32_response blkif_x86_32_response_t;
>> >> >>  #pragma pack(pop)
>> >> >>  
>> >> >>  /* x86_64 protocol version */
>> >> >> @@ -62,20 +53,14 @@ struct blkif_x86_64_request_discard {
>> >> >>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w 
>> > only)  */
>> >> >>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard 
>> >   */
>> >> >>  };
>> >> >> -struct blkif_x86_64_response {
>> >> >> -    uint64_t       __attribute__((__aligned__(8))) id;
>> >> >> -    uint8_t         operation;       /* copied from request */
>> >> >> -    int16_t         status;          /* BLKIF_RSP_???       */
>> >> >> -};
>> >> >>
>> >> >>  typedef struct blkif_x86_64_request blkif_x86_64_request_t;
>> >> >> -typedef struct blkif_x86_64_response blkif_x86_64_response_t;
>> >> >>  
>> >> >>  DEFINE_RING_TYPES(blkif_common, struct blkif_common_request,
>> >> >> -                  struct blkif_common_response);
>> >> >> +                  struct blkif_response);
>> >> >>  DEFINE_RING_TYPES(blkif_x86_32, struct blkif_x86_32_request,
>> >> >> -                  struct blkif_x86_32_response);
>> >> >> +                  struct blkif_response QEMU_PACKED);
>> >> > 
>> >> > In my test, the previous sizes and alignments of the response structs
>> >> > were (on both x86_32 and x86_64):
>> >> > 
>> >> > sizeof(blkif_x86_32_response)=12   sizeof(blkif_x86_64_response)=16
>> >> > align(blkif_x86_32_response)=4     align(blkif_x86_64_response)=8
>> >> > 
>> >> > While with these changes are now, when compiled on x86_64:
>> >> > sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=16
>> >> > align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=8
>> >> > 
>> >> > when compiled on x86_32:
>> >> > sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=12
>> >> > align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=4
>> >> > 
>> >> > Did I do my tests wrong?
>> >> > 
>> >> > QEMU_PACKED is not the same as #pragma pack(push, 4). In fact, it is the
>> >> > same as #pragma pack(push, 1), causing the struct to be densely packed,
>> >> > leaving no padding whatsever.
>> >> > 
>> >> > In addition, without __attribute__((__aligned__(8))),
>> >> > blkif_x86_64_response won't be 8 bytes aligned when built on x86_32.
>> >> > 
>> >> > Am I missing something?
>> >> 
>> >> Well, you're mixing attribute application upon structure
>> >> declaration with attribute application upon structure use. It's
>> >> the latter here, and hence the attribute doesn't affect
>> >> structure layout at all. All it does is avoid the _containing_
>> >> 32-bit union to become 8-byte aligned (and tail padding to be
>> >> inserted).
>> > 
>> > Thanks for the explanation. I admit it's the first time I see the
>> > aligned attribute being used at structure usage only. I think it's the
>> > first time QEMU_PACKED is used this way in QEMU too.
>> > 
>> > Anyway, even taking that into account, things are still not completely
>> > right: the alignment of struct blkif_x86_32_response QEMU_PACKED is 4
>> > bytes as you wrote, but the size of struct blkif_x86_32_response is
>> > still 16 bytes instead of 12 bytes in my test. I suspect it worked for
>> > you because the other member of the union (blkif_x86_32_request) is
>> > larger than that. However, I think is not a good idea to rely on this
>> > implementation detail. The implementation of DEFINE_RING_TYPES should be
>> > opaque from our point of view. We shouldn't have to know that there is a
>> > union there.
>> 
>> I don't follow - why should we not rely on this? It is a fundamental
>> aspect of the shared ring model that requests and responses share
>> space.
>> 
>> > Moreover, the other problem is still unaddressed: the size and alignment
>> > of blkif_x86_64_response when built on x86_32 are 12 and 4 instead of 16
>> > and 8 bytes. Is that working also because it's relying on the other
>> > member of the union to enforce the right alignment and bigger size?
>> 
>> Yes. For these as well as your comments further up - sizeof() and
>> alignof() are completely uninteresting as long as we don't
>> instantiate objects of those types _and then use them for
>> communication_. The patch specifically removes instantiation,
>> switching to a purely pointer based approach. And that is ...
> 
> As long as we don't instantiate objects of those types and we use them
> for communication? This is basically a death trap hidden in an innocuous
> looking header file.

I'm sorry, but - what? Neither frontend nor backend are supposed
to have local instantiations of the structure _and_ use those for
communication. Doing just one of the two is fine, with the caveat
of risking to re-introduce the issue being fixed here if done so in a
backend. Of course the same would apply to request structures
used by a frontend if it cares to not leak information to the
backend.

To outright prevent such issues to occur, it would be a good thing
if the structures could be marked such that the compiler would
refuse to create instantiations (i.e. only pointers to the structure
would be allowed). But since we'd have to arrange for that at
structure declaration point, and since standard C89 doesn't have
ways to arrange for that (and I'd have to see whether there's a
gcc or C99 extension allowing to do so, as I can't think of one
right away), we simply can't do so.

>> In the end I'm surprised the qemu side is proving so much more
>> difficult to get accepted compared to the Linux one.
> 
> I am happy to write the code and/or the commit message. Would a simple
> cast like below work to fix the security issue?

I suppose so, but you do realize that this is _exactly_ what
my patch does, except you use dangerous casts while I play
type-safe?

Jan

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

Re: [Qemu-devel] [PATCH] xen/disk: don't leak stack data via response ring

[Stefano Stabellini]

On Fri, 23 Jun 2017, Jan Beulich wrote:
> >>> On 22.06.17 at 20:52, <sstabellini@kernel.org> wrote:
> > On Thu, 22 Jun 2017, Jan Beulich wrote:
> >> >>> On 21.06.17 at 20:46, <sstabellini@kernel.org> wrote:
> >> > On Wed, 21 Jun 2017, Jan Beulich wrote:
> >> >> >>> On 20.06.17 at 23:48, <sstabellini@kernel.org> wrote:
> >> >> > On Tue, 20 Jun 2017, Jan Beulich wrote:
> >> >> >> @@ -36,13 +33,7 @@ struct blkif_x86_32_request_discard {
> >> >> >>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w 
> >> > only)  */
> >> >> >>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard 
> >> >   */
> >> >> >>  };
> >> >> >> -struct blkif_x86_32_response {
> >> >> >> -    uint64_t        id;              /* copied from request */
> >> >> >> -    uint8_t         operation;       /* copied from request */
> >> >> >> -    int16_t         status;          /* BLKIF_RSP_???       */
> >> >> >> -};
> >> >> >>  typedef struct blkif_x86_32_request blkif_x86_32_request_t;
> >> >> >> -typedef struct blkif_x86_32_response blkif_x86_32_response_t;
> >> >> >>  #pragma pack(pop)
> >> >> >>  
> >> >> >>  /* x86_64 protocol version */
> >> >> >> @@ -62,20 +53,14 @@ struct blkif_x86_64_request_discard {
> >> >> >>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w 
> >> > only)  */
> >> >> >>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard 
> >> >   */
> >> >> >>  };
> >> >> >> -struct blkif_x86_64_response {
> >> >> >> -    uint64_t       __attribute__((__aligned__(8))) id;
> >> >> >> -    uint8_t         operation;       /* copied from request */
> >> >> >> -    int16_t         status;          /* BLKIF_RSP_???       */
> >> >> >> -};
> >> >> >>
> >> >> >>  typedef struct blkif_x86_64_request blkif_x86_64_request_t;
> >> >> >> -typedef struct blkif_x86_64_response blkif_x86_64_response_t;
> >> >> >>  
> >> >> >>  DEFINE_RING_TYPES(blkif_common, struct blkif_common_request,
> >> >> >> -                  struct blkif_common_response);
> >> >> >> +                  struct blkif_response);
> >> >> >>  DEFINE_RING_TYPES(blkif_x86_32, struct blkif_x86_32_request,
> >> >> >> -                  struct blkif_x86_32_response);
> >> >> >> +                  struct blkif_response QEMU_PACKED);
> >> >> > 
> >> >> > In my test, the previous sizes and alignments of the response structs
> >> >> > were (on both x86_32 and x86_64):
> >> >> > 
> >> >> > sizeof(blkif_x86_32_response)=12   sizeof(blkif_x86_64_response)=16
> >> >> > align(blkif_x86_32_response)=4     align(blkif_x86_64_response)=8
> >> >> > 
> >> >> > While with these changes are now, when compiled on x86_64:
> >> >> > sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=16
> >> >> > align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=8
> >> >> > 
> >> >> > when compiled on x86_32:
> >> >> > sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=12
> >> >> > align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=4
> >> >> > 
> >> >> > Did I do my tests wrong?
> >> >> > 
> >> >> > QEMU_PACKED is not the same as #pragma pack(push, 4). In fact, it is the
> >> >> > same as #pragma pack(push, 1), causing the struct to be densely packed,
> >> >> > leaving no padding whatsever.
> >> >> > 
> >> >> > In addition, without __attribute__((__aligned__(8))),
> >> >> > blkif_x86_64_response won't be 8 bytes aligned when built on x86_32.
> >> >> > 
> >> >> > Am I missing something?
> >> >> 
> >> >> Well, you're mixing attribute application upon structure
> >> >> declaration with attribute application upon structure use. It's
> >> >> the latter here, and hence the attribute doesn't affect
> >> >> structure layout at all. All it does is avoid the _containing_
> >> >> 32-bit union to become 8-byte aligned (and tail padding to be
> >> >> inserted).
> >> > 
> >> > Thanks for the explanation. I admit it's the first time I see the
> >> > aligned attribute being used at structure usage only. I think it's the
> >> > first time QEMU_PACKED is used this way in QEMU too.
> >> > 
> >> > Anyway, even taking that into account, things are still not completely
> >> > right: the alignment of struct blkif_x86_32_response QEMU_PACKED is 4
> >> > bytes as you wrote, but the size of struct blkif_x86_32_response is
> >> > still 16 bytes instead of 12 bytes in my test. I suspect it worked for
> >> > you because the other member of the union (blkif_x86_32_request) is
> >> > larger than that. However, I think is not a good idea to rely on this
> >> > implementation detail. The implementation of DEFINE_RING_TYPES should be
> >> > opaque from our point of view. We shouldn't have to know that there is a
> >> > union there.
> >> 
> >> I don't follow - why should we not rely on this? It is a fundamental
> >> aspect of the shared ring model that requests and responses share
> >> space.
> >> 
> >> > Moreover, the other problem is still unaddressed: the size and alignment
> >> > of blkif_x86_64_response when built on x86_32 are 12 and 4 instead of 16
> >> > and 8 bytes. Is that working also because it's relying on the other
> >> > member of the union to enforce the right alignment and bigger size?
> >> 
> >> Yes. For these as well as your comments further up - sizeof() and
> >> alignof() are completely uninteresting as long as we don't
> >> instantiate objects of those types _and then use them for
> >> communication_. The patch specifically removes instantiation,
> >> switching to a purely pointer based approach. And that is ...
> > 
> > As long as we don't instantiate objects of those types and we use them
> > for communication? This is basically a death trap hidden in an innocuous
> > looking header file.
> 
> I'm sorry, but - what? Neither frontend nor backend are supposed
> to have local instantiations of the structure _and_ use those for
> communication. Doing just one of the two is fine, with the caveat
> of risking to re-introduce the issue being fixed here if done so in a
> backend. Of course the same would apply to request structures
> used by a frontend if it cares to not leak information to the
> backend.
> 
> To outright prevent such issues to occur, it would be a good thing
> if the structures could be marked such that the compiler would
> refuse to create instantiations (i.e. only pointers to the structure
> would be allowed). But since we'd have to arrange for that at
> structure declaration point, and since standard C89 doesn't have
> ways to arrange for that (and I'd have to see whether there's a
> gcc or C99 extension allowing to do so, as I can't think of one
> right away), we simply can't do so.

Yes, you are right that if we could, we would do that. It would make it
safer.


> >> In the end I'm surprised the qemu side is proving so much more
> >> difficult to get accepted compared to the Linux one.
> > 
> > I am happy to write the code and/or the commit message. Would a simple
> > cast like below work to fix the security issue?
> 
> I suppose so, but you do realize that this is _exactly_ what
> my patch does, except you use dangerous casts while I play
> type-safe?

Why is the cast dangerous? Both your patch and my version of it rely on
inner knowledge of the way the rings are laid out in memory, but at
least my patch doesn't introduce the risk of instantiating broken
structs. Besides, type safety checks don't add much value, given that we
rely on the way the ring.h macros have been written, which weren't even
imported in the QEMU project until March this year.

These are the reasons why I prefer my version, but I would consider your
version with clear in-code comments that warn users to avoid
instantiating the structs because they are not ABI compliant.

How do you want to proceed?

Re: [PATCH] xen/disk: don't leak stack data via response ring

[Stefano Stabellini]

On Fri, 23 Jun 2017, Jan Beulich wrote:
> >>> On 22.06.17 at 20:52, <sstabellini@kernel.org> wrote:
> > On Thu, 22 Jun 2017, Jan Beulich wrote:
> >> >>> On 21.06.17 at 20:46, <sstabellini@kernel.org> wrote:
> >> > On Wed, 21 Jun 2017, Jan Beulich wrote:
> >> >> >>> On 20.06.17 at 23:48, <sstabellini@kernel.org> wrote:
> >> >> > On Tue, 20 Jun 2017, Jan Beulich wrote:
> >> >> >> @@ -36,13 +33,7 @@ struct blkif_x86_32_request_discard {
> >> >> >>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w 
> >> > only)  */
> >> >> >>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard 
> >> >   */
> >> >> >>  };
> >> >> >> -struct blkif_x86_32_response {
> >> >> >> -    uint64_t        id;              /* copied from request */
> >> >> >> -    uint8_t         operation;       /* copied from request */
> >> >> >> -    int16_t         status;          /* BLKIF_RSP_???       */
> >> >> >> -};
> >> >> >>  typedef struct blkif_x86_32_request blkif_x86_32_request_t;
> >> >> >> -typedef struct blkif_x86_32_response blkif_x86_32_response_t;
> >> >> >>  #pragma pack(pop)
> >> >> >>  
> >> >> >>  /* x86_64 protocol version */
> >> >> >> @@ -62,20 +53,14 @@ struct blkif_x86_64_request_discard {
> >> >> >>      blkif_sector_t sector_number;    /* start sector idx on disk (r/w 
> >> > only)  */
> >> >> >>      uint64_t       nr_sectors;       /* # of contiguous sectors to discard 
> >> >   */
> >> >> >>  };
> >> >> >> -struct blkif_x86_64_response {
> >> >> >> -    uint64_t       __attribute__((__aligned__(8))) id;
> >> >> >> -    uint8_t         operation;       /* copied from request */
> >> >> >> -    int16_t         status;          /* BLKIF_RSP_???       */
> >> >> >> -};
> >> >> >>
> >> >> >>  typedef struct blkif_x86_64_request blkif_x86_64_request_t;
> >> >> >> -typedef struct blkif_x86_64_response blkif_x86_64_response_t;
> >> >> >>  
> >> >> >>  DEFINE_RING_TYPES(blkif_common, struct blkif_common_request,
> >> >> >> -                  struct blkif_common_response);
> >> >> >> +                  struct blkif_response);
> >> >> >>  DEFINE_RING_TYPES(blkif_x86_32, struct blkif_x86_32_request,
> >> >> >> -                  struct blkif_x86_32_response);
> >> >> >> +                  struct blkif_response QEMU_PACKED);
> >> >> > 
> >> >> > In my test, the previous sizes and alignments of the response structs
> >> >> > were (on both x86_32 and x86_64):
> >> >> > 
> >> >> > sizeof(blkif_x86_32_response)=12   sizeof(blkif_x86_64_response)=16
> >> >> > align(blkif_x86_32_response)=4     align(blkif_x86_64_response)=8
> >> >> > 
> >> >> > While with these changes are now, when compiled on x86_64:
> >> >> > sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=16
> >> >> > align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=8
> >> >> > 
> >> >> > when compiled on x86_32:
> >> >> > sizeof(blkif_x86_32_response)=11   sizeof(blkif_x86_64_response)=12
> >> >> > align(blkif_x86_32_response)=1     align(blkif_x86_64_response)=4
> >> >> > 
> >> >> > Did I do my tests wrong?
> >> >> > 
> >> >> > QEMU_PACKED is not the same as #pragma pack(push, 4). In fact, it is the
> >> >> > same as #pragma pack(push, 1), causing the struct to be densely packed,
> >> >> > leaving no padding whatsever.
> >> >> > 
> >> >> > In addition, without __attribute__((__aligned__(8))),
> >> >> > blkif_x86_64_response won't be 8 bytes aligned when built on x86_32.
> >> >> > 
> >> >> > Am I missing something?
> >> >> 
> >> >> Well, you're mixing attribute application upon structure
> >> >> declaration with attribute application upon structure use. It's
> >> >> the latter here, and hence the attribute doesn't affect
> >> >> structure layout at all. All it does is avoid the _containing_
> >> >> 32-bit union to become 8-byte aligned (and tail padding to be
> >> >> inserted).
> >> > 
> >> > Thanks for the explanation. I admit it's the first time I see the
> >> > aligned attribute being used at structure usage only. I think it's the
> >> > first time QEMU_PACKED is used this way in QEMU too.
> >> > 
> >> > Anyway, even taking that into account, things are still not completely
> >> > right: the alignment of struct blkif_x86_32_response QEMU_PACKED is 4
> >> > bytes as you wrote, but the size of struct blkif_x86_32_response is
> >> > still 16 bytes instead of 12 bytes in my test. I suspect it worked for
> >> > you because the other member of the union (blkif_x86_32_request) is
> >> > larger than that. However, I think is not a good idea to rely on this
> >> > implementation detail. The implementation of DEFINE_RING_TYPES should be
> >> > opaque from our point of view. We shouldn't have to know that there is a
> >> > union there.
> >> 
> >> I don't follow - why should we not rely on this? It is a fundamental
> >> aspect of the shared ring model that requests and responses share
> >> space.
> >> 
> >> > Moreover, the other problem is still unaddressed: the size and alignment
> >> > of blkif_x86_64_response when built on x86_32 are 12 and 4 instead of 16
> >> > and 8 bytes. Is that working also because it's relying on the other
> >> > member of the union to enforce the right alignment and bigger size?
> >> 
> >> Yes. For these as well as your comments further up - sizeof() and
> >> alignof() are completely uninteresting as long as we don't
> >> instantiate objects of those types _and then use them for
> >> communication_. The patch specifically removes instantiation,
> >> switching to a purely pointer based approach. And that is ...
> > 
> > As long as we don't instantiate objects of those types and we use them
> > for communication? This is basically a death trap hidden in an innocuous
> > looking header file.
> 
> I'm sorry, but - what? Neither frontend nor backend are supposed
> to have local instantiations of the structure _and_ use those for
> communication. Doing just one of the two is fine, with the caveat
> of risking to re-introduce the issue being fixed here if done so in a
> backend. Of course the same would apply to request structures
> used by a frontend if it cares to not leak information to the
> backend.
> 
> To outright prevent such issues to occur, it would be a good thing
> if the structures could be marked such that the compiler would
> refuse to create instantiations (i.e. only pointers to the structure
> would be allowed). But since we'd have to arrange for that at
> structure declaration point, and since standard C89 doesn't have
> ways to arrange for that (and I'd have to see whether there's a
> gcc or C99 extension allowing to do so, as I can't think of one
> right away), we simply can't do so.

Yes, you are right that if we could, we would do that. It would make it
safer.


> >> In the end I'm surprised the qemu side is proving so much more
> >> difficult to get accepted compared to the Linux one.
> > 
> > I am happy to write the code and/or the commit message. Would a simple
> > cast like below work to fix the security issue?
> 
> I suppose so, but you do realize that this is _exactly_ what
> my patch does, except you use dangerous casts while I play
> type-safe?

Why is the cast dangerous? Both your patch and my version of it rely on
inner knowledge of the way the rings are laid out in memory, but at
least my patch doesn't introduce the risk of instantiating broken
structs. Besides, type safety checks don't add much value, given that we
rely on the way the ring.h macros have been written, which weren't even
imported in the QEMU project until March this year.

These are the reasons why I prefer my version, but I would consider your
version with clear in-code comments that warn users to avoid
instantiating the structs because they are not ABI compliant.

How do you want to proceed?

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

Re: [Qemu-devel] [PATCH] xen/disk: don't leak stack data via response ring

[Jan Beulich]

>>> Stefano Stabellini <sstabellini@kernel.org> 06/23/17 8:43 PM >>>
>On Fri, 23 Jun 2017, Jan Beulich wrote:
>> >>> On 22.06.17 at 20:52, <sstabellini@kernel.org> wrote:
>> > I am happy to write the code and/or the commit message. Would a simple
>> > cast like below work to fix the security issue?
>> 
>> I suppose so, but you do realize that this is _exactly_ what
>> my patch does, except you use dangerous casts while I play
>> type-safe?
>
>Why is the cast dangerous?

Casts, and especially pointer ones) are always dangerous, as they have
the potential of type changes elsewhere going unnoticed. You may have
noticed that in reviews I'm often picking at casts, because in a majority
of cases people use them they're not needed and hence their use is a
(latent) risk.

> Both your patch and my version of it rely on
>inner knowledge of the way the rings are laid out in memory, but at
>least my patch doesn't introduce the risk of instantiating broken
>structs. Besides, type safety checks don't add much value, given that we
>rely on the way the ring.h macros have been written, which weren't even
>imported in the QEMU project until March this year.

That's said, as it seems to imply backporting of the change to older
qemu may then be problematic. Otoh I don't recall having had problems
with using the patch with only minor adjustments on older trees of ours.

>These are the reasons why I prefer my version, but I would consider your
>version with clear in-code comments that warn users to avoid
>instantiating the structs because they are not ABI compliant.
>
>How do you want to proceed?

I can provide a version with comments added, but only next week. If you
feel like you want to go with your variant, I won't stand in the way, but I
also wouldn't give it my ack or alike (which you don't depend on anyway).

Jan

Re: [PATCH] xen/disk: don't leak stack data via response ring

[Jan Beulich]

>>> Stefano Stabellini <sstabellini@kernel.org> 06/23/17 8:43 PM >>>
>On Fri, 23 Jun 2017, Jan Beulich wrote:
>> >>> On 22.06.17 at 20:52, <sstabellini@kernel.org> wrote:
>> > I am happy to write the code and/or the commit message. Would a simple
>> > cast like below work to fix the security issue?
>> 
>> I suppose so, but you do realize that this is _exactly_ what
>> my patch does, except you use dangerous casts while I play
>> type-safe?
>
>Why is the cast dangerous?

Casts, and especially pointer ones) are always dangerous, as they have
the potential of type changes elsewhere going unnoticed. You may have
noticed that in reviews I'm often picking at casts, because in a majority
of cases people use them they're not needed and hence their use is a
(latent) risk.

> Both your patch and my version of it rely on
>inner knowledge of the way the rings are laid out in memory, but at
>least my patch doesn't introduce the risk of instantiating broken
>structs. Besides, type safety checks don't add much value, given that we
>rely on the way the ring.h macros have been written, which weren't even
>imported in the QEMU project until March this year.

That's said, as it seems to imply backporting of the change to older
qemu may then be problematic. Otoh I don't recall having had problems
with using the patch with only minor adjustments on older trees of ours.

>These are the reasons why I prefer my version, but I would consider your
>version with clear in-code comments that warn users to avoid
>instantiating the structs because they are not ABI compliant.
>
>How do you want to proceed?

I can provide a version with comments added, but only next week. If you
feel like you want to go with your variant, I won't stand in the way, but I
also wouldn't give it my ack or alike (which you don't depend on anyway).

Jan


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

Re: [Qemu-devel] [PATCH] xen/disk: don't leak stack data via response ring

[Stefano Stabellini]

On Mon, 26 Jun 2017, Jan Beulich wrote:
> >>> Stefano Stabellini <sstabellini@kernel.org> 06/23/17 8:43 PM >>>
> >On Fri, 23 Jun 2017, Jan Beulich wrote:
> >> >>> On 22.06.17 at 20:52, <sstabellini@kernel.org> wrote:
> >> > I am happy to write the code and/or the commit message. Would a simple
> >> > cast like below work to fix the security issue?
> >> 
> >> I suppose so, but you do realize that this is _exactly_ what
> >> my patch does, except you use dangerous casts while I play
> >> type-safe?
> >
> >Why is the cast dangerous?
> 
> Casts, and especially pointer ones) are always dangerous, as they have
> the potential of type changes elsewhere going unnoticed. You may have
> noticed that in reviews I'm often picking at casts, because in a majority
> of cases people use them they're not needed and hence their use is a
> (latent) risk.
> 
> > Both your patch and my version of it rely on
> >inner knowledge of the way the rings are laid out in memory, but at
> >least my patch doesn't introduce the risk of instantiating broken
> >structs. Besides, type safety checks don't add much value, given that we
> >rely on the way the ring.h macros have been written, which weren't even
> >imported in the QEMU project until March this year.
> 
> That's said, as it seems to imply backporting of the change to older
> qemu may then be problematic. Otoh I don't recall having had problems
> with using the patch with only minor adjustments on older trees of ours.
> 
> >These are the reasons why I prefer my version, but I would consider your
> >version with clear in-code comments that warn users to avoid
> >instantiating the structs because they are not ABI compliant.
> >
> >How do you want to proceed?
> 
> I can provide a version with comments added, but only next week. If you
> feel like you want to go with your variant, I won't stand in the way, but I
> also wouldn't give it my ack or alike (which you don't depend on anyway).

After careful consideration, I think I'll submit my version of the
patch, assuming that Anthony is OK with it. (FYI I had to keep your
signed-off-by line for copyright reasons: you own the copyright on some
of the changes.)

Anthony, what do you think of the following:

---

xen/disk: don't leak stack data via response ring

Rather than constructing a local structure instance on the stack, fill
the fields directly on the shared ring, just like other (Linux)
backends do. Build on the fact that all response structure flavors are
actually identical (aside from alignment and padding at the end).

This is XSA-216.

Reported by: Anthony Perard <anthony.perard@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>

diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 3a22805..9200511 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -769,31 +769,30 @@ static int blk_send_response_one(struct ioreq *ioreq)
     struct XenBlkDev  *blkdev = ioreq->blkdev;
     int               send_notify   = 0;
     int               have_requests = 0;
-    blkif_response_t  resp;
-    void              *dst;
-
-    resp.id        = ioreq->req.id;
-    resp.operation = ioreq->req.operation;
-    resp.status    = ioreq->status;
+    blkif_response_t  *resp;
 
     /* Place on the response ring for the relevant domain. */
     switch (blkdev->protocol) {
     case BLKIF_PROTOCOL_NATIVE:
-        dst = RING_GET_RESPONSE(&blkdev->rings.native, blkdev->rings.native.rsp_prod_pvt);
+        resp = (blkif_response_t *) RING_GET_RESPONSE(&blkdev->rings.native,
+                                 blkdev->rings.native.rsp_prod_pvt);
         break;
     case BLKIF_PROTOCOL_X86_32:
-        dst = RING_GET_RESPONSE(&blkdev->rings.x86_32_part,
-                                blkdev->rings.x86_32_part.rsp_prod_pvt);
+        resp = (blkif_response_t *) RING_GET_RESPONSE(&blkdev->rings.x86_32_part,
+                                 blkdev->rings.x86_32_part.rsp_prod_pvt);
         break;
     case BLKIF_PROTOCOL_X86_64:
-        dst = RING_GET_RESPONSE(&blkdev->rings.x86_64_part,
-                                blkdev->rings.x86_64_part.rsp_prod_pvt);
+        resp = (blkif_response_t *) RING_GET_RESPONSE(&blkdev->rings.x86_64_part,
+                                 blkdev->rings.x86_64_part.rsp_prod_pvt);
         break;
     default:
-        dst = NULL;
         return 0;
     }
-    memcpy(dst, &resp, sizeof(resp));
+
+    resp->id        = ioreq->req.id;
+    resp->operation = ioreq->req.operation;
+    resp->status    = ioreq->status;
+
     blkdev->rings.common.rsp_prod_pvt++;
 
     RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&blkdev->rings.common, send_notify);

Re: [PATCH] xen/disk: don't leak stack data via response ring

[Stefano Stabellini]

On Mon, 26 Jun 2017, Jan Beulich wrote:
> >>> Stefano Stabellini <sstabellini@kernel.org> 06/23/17 8:43 PM >>>
> >On Fri, 23 Jun 2017, Jan Beulich wrote:
> >> >>> On 22.06.17 at 20:52, <sstabellini@kernel.org> wrote:
> >> > I am happy to write the code and/or the commit message. Would a simple
> >> > cast like below work to fix the security issue?
> >> 
> >> I suppose so, but you do realize that this is _exactly_ what
> >> my patch does, except you use dangerous casts while I play
> >> type-safe?
> >
> >Why is the cast dangerous?
> 
> Casts, and especially pointer ones) are always dangerous, as they have
> the potential of type changes elsewhere going unnoticed. You may have
> noticed that in reviews I'm often picking at casts, because in a majority
> of cases people use them they're not needed and hence their use is a
> (latent) risk.
> 
> > Both your patch and my version of it rely on
> >inner knowledge of the way the rings are laid out in memory, but at
> >least my patch doesn't introduce the risk of instantiating broken
> >structs. Besides, type safety checks don't add much value, given that we
> >rely on the way the ring.h macros have been written, which weren't even
> >imported in the QEMU project until March this year.
> 
> That's said, as it seems to imply backporting of the change to older
> qemu may then be problematic. Otoh I don't recall having had problems
> with using the patch with only minor adjustments on older trees of ours.
> 
> >These are the reasons why I prefer my version, but I would consider your
> >version with clear in-code comments that warn users to avoid
> >instantiating the structs because they are not ABI compliant.
> >
> >How do you want to proceed?
> 
> I can provide a version with comments added, but only next week. If you
> feel like you want to go with your variant, I won't stand in the way, but I
> also wouldn't give it my ack or alike (which you don't depend on anyway).

After careful consideration, I think I'll submit my version of the
patch, assuming that Anthony is OK with it. (FYI I had to keep your
signed-off-by line for copyright reasons: you own the copyright on some
of the changes.)

Anthony, what do you think of the following:

---

xen/disk: don't leak stack data via response ring

Rather than constructing a local structure instance on the stack, fill
the fields directly on the shared ring, just like other (Linux)
backends do. Build on the fact that all response structure flavors are
actually identical (aside from alignment and padding at the end).

This is XSA-216.

Reported by: Anthony Perard <anthony.perard@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>

diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 3a22805..9200511 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -769,31 +769,30 @@ static int blk_send_response_one(struct ioreq *ioreq)
     struct XenBlkDev  *blkdev = ioreq->blkdev;
     int               send_notify   = 0;
     int               have_requests = 0;
-    blkif_response_t  resp;
-    void              *dst;
-
-    resp.id        = ioreq->req.id;
-    resp.operation = ioreq->req.operation;
-    resp.status    = ioreq->status;
+    blkif_response_t  *resp;
 
     /* Place on the response ring for the relevant domain. */
     switch (blkdev->protocol) {
     case BLKIF_PROTOCOL_NATIVE:
-        dst = RING_GET_RESPONSE(&blkdev->rings.native, blkdev->rings.native.rsp_prod_pvt);
+        resp = (blkif_response_t *) RING_GET_RESPONSE(&blkdev->rings.native,
+                                 blkdev->rings.native.rsp_prod_pvt);
         break;
     case BLKIF_PROTOCOL_X86_32:
-        dst = RING_GET_RESPONSE(&blkdev->rings.x86_32_part,
-                                blkdev->rings.x86_32_part.rsp_prod_pvt);
+        resp = (blkif_response_t *) RING_GET_RESPONSE(&blkdev->rings.x86_32_part,
+                                 blkdev->rings.x86_32_part.rsp_prod_pvt);
         break;
     case BLKIF_PROTOCOL_X86_64:
-        dst = RING_GET_RESPONSE(&blkdev->rings.x86_64_part,
-                                blkdev->rings.x86_64_part.rsp_prod_pvt);
+        resp = (blkif_response_t *) RING_GET_RESPONSE(&blkdev->rings.x86_64_part,
+                                 blkdev->rings.x86_64_part.rsp_prod_pvt);
         break;
     default:
-        dst = NULL;
         return 0;
     }
-    memcpy(dst, &resp, sizeof(resp));
+
+    resp->id        = ioreq->req.id;
+    resp->operation = ioreq->req.operation;
+    resp->status    = ioreq->status;
+
     blkdev->rings.common.rsp_prod_pvt++;
 
     RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&blkdev->rings.common, send_notify);

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

Re: [Qemu-devel] [PATCH] xen/disk: don't leak stack data via response ring

[Anthony PERARD]

On Mon, Jun 26, 2017 at 12:12:18PM -0700, Stefano Stabellini wrote:
> On Mon, 26 Jun 2017, Jan Beulich wrote:
> > >>> Stefano Stabellini <sstabellini@kernel.org> 06/23/17 8:43 PM >>>
> > >On Fri, 23 Jun 2017, Jan Beulich wrote:
> > >> >>> On 22.06.17 at 20:52, <sstabellini@kernel.org> wrote:
> > >> > I am happy to write the code and/or the commit message. Would a simple
> > >> > cast like below work to fix the security issue?
> > >> 
> > >> I suppose so, but you do realize that this is _exactly_ what
> > >> my patch does, except you use dangerous casts while I play
> > >> type-safe?
> > >
> > >Why is the cast dangerous?
> > 
> > Casts, and especially pointer ones) are always dangerous, as they have
> > the potential of type changes elsewhere going unnoticed. You may have
> > noticed that in reviews I'm often picking at casts, because in a majority
> > of cases people use them they're not needed and hence their use is a
> > (latent) risk.
> > 
> > > Both your patch and my version of it rely on
> > >inner knowledge of the way the rings are laid out in memory, but at
> > >least my patch doesn't introduce the risk of instantiating broken
> > >structs. Besides, type safety checks don't add much value, given that we
> > >rely on the way the ring.h macros have been written, which weren't even
> > >imported in the QEMU project until March this year.
> > 
> > That's said, as it seems to imply backporting of the change to older
> > qemu may then be problematic. Otoh I don't recall having had problems
> > with using the patch with only minor adjustments on older trees of ours.
> > 
> > >These are the reasons why I prefer my version, but I would consider your
> > >version with clear in-code comments that warn users to avoid
> > >instantiating the structs because they are not ABI compliant.
> > >
> > >How do you want to proceed?
> > 
> > I can provide a version with comments added, but only next week. If you
> > feel like you want to go with your variant, I won't stand in the way, but I
> > also wouldn't give it my ack or alike (which you don't depend on anyway).
> 
> After careful consideration, I think I'll submit my version of the
> patch, assuming that Anthony is OK with it. (FYI I had to keep your
> signed-off-by line for copyright reasons: you own the copyright on some
> of the changes.)
> 
> Anthony, what do you think of the following:

I do prefare the version without cast, but this is fine too. It is much
easier to understand that nothing changes beside the fact that field are
filled directly on the shared ring rather than from the stack. So I'm
fine with it.

You can add my
Acked-by: Anthony PERARD <anthony.perard@citrix.com>

> ---
> 
> xen/disk: don't leak stack data via response ring
> 
> Rather than constructing a local structure instance on the stack, fill
> the fields directly on the shared ring, just like other (Linux)
> backends do. Build on the fact that all response structure flavors are
> actually identical (aside from alignment and padding at the end).
> 
> This is XSA-216.
> 
> Reported by: Anthony Perard <anthony.perard@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 3a22805..9200511 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -769,31 +769,30 @@ static int blk_send_response_one(struct ioreq *ioreq)
>      struct XenBlkDev  *blkdev = ioreq->blkdev;
>      int               send_notify   = 0;
>      int               have_requests = 0;
> -    blkif_response_t  resp;
> -    void              *dst;
> -
> -    resp.id        = ioreq->req.id;
> -    resp.operation = ioreq->req.operation;
> -    resp.status    = ioreq->status;
> +    blkif_response_t  *resp;
>  
>      /* Place on the response ring for the relevant domain. */
>      switch (blkdev->protocol) {
>      case BLKIF_PROTOCOL_NATIVE:
> -        dst = RING_GET_RESPONSE(&blkdev->rings.native, blkdev->rings.native.rsp_prod_pvt);
> +        resp = (blkif_response_t *) RING_GET_RESPONSE(&blkdev->rings.native,
> +                                 blkdev->rings.native.rsp_prod_pvt);
>          break;
>      case BLKIF_PROTOCOL_X86_32:
> -        dst = RING_GET_RESPONSE(&blkdev->rings.x86_32_part,
> -                                blkdev->rings.x86_32_part.rsp_prod_pvt);
> +        resp = (blkif_response_t *) RING_GET_RESPONSE(&blkdev->rings.x86_32_part,
> +                                 blkdev->rings.x86_32_part.rsp_prod_pvt);
>          break;
>      case BLKIF_PROTOCOL_X86_64:
> -        dst = RING_GET_RESPONSE(&blkdev->rings.x86_64_part,
> -                                blkdev->rings.x86_64_part.rsp_prod_pvt);
> +        resp = (blkif_response_t *) RING_GET_RESPONSE(&blkdev->rings.x86_64_part,
> +                                 blkdev->rings.x86_64_part.rsp_prod_pvt);
>          break;
>      default:
> -        dst = NULL;
>          return 0;
>      }
> -    memcpy(dst, &resp, sizeof(resp));
> +
> +    resp->id        = ioreq->req.id;
> +    resp->operation = ioreq->req.operation;
> +    resp->status    = ioreq->status;
> +
>      blkdev->rings.common.rsp_prod_pvt++;
>  
>      RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&blkdev->rings.common, send_notify);

-- 
Anthony PERARD

Re: [PATCH] xen/disk: don't leak stack data via response ring

[Anthony PERARD]

On Mon, Jun 26, 2017 at 12:12:18PM -0700, Stefano Stabellini wrote:
> On Mon, 26 Jun 2017, Jan Beulich wrote:
> > >>> Stefano Stabellini <sstabellini@kernel.org> 06/23/17 8:43 PM >>>
> > >On Fri, 23 Jun 2017, Jan Beulich wrote:
> > >> >>> On 22.06.17 at 20:52, <sstabellini@kernel.org> wrote:
> > >> > I am happy to write the code and/or the commit message. Would a simple
> > >> > cast like below work to fix the security issue?
> > >> 
> > >> I suppose so, but you do realize that this is _exactly_ what
> > >> my patch does, except you use dangerous casts while I play
> > >> type-safe?
> > >
> > >Why is the cast dangerous?
> > 
> > Casts, and especially pointer ones) are always dangerous, as they have
> > the potential of type changes elsewhere going unnoticed. You may have
> > noticed that in reviews I'm often picking at casts, because in a majority
> > of cases people use them they're not needed and hence their use is a
> > (latent) risk.
> > 
> > > Both your patch and my version of it rely on
> > >inner knowledge of the way the rings are laid out in memory, but at
> > >least my patch doesn't introduce the risk of instantiating broken
> > >structs. Besides, type safety checks don't add much value, given that we
> > >rely on the way the ring.h macros have been written, which weren't even
> > >imported in the QEMU project until March this year.
> > 
> > That's said, as it seems to imply backporting of the change to older
> > qemu may then be problematic. Otoh I don't recall having had problems
> > with using the patch with only minor adjustments on older trees of ours.
> > 
> > >These are the reasons why I prefer my version, but I would consider your
> > >version with clear in-code comments that warn users to avoid
> > >instantiating the structs because they are not ABI compliant.
> > >
> > >How do you want to proceed?
> > 
> > I can provide a version with comments added, but only next week. If you
> > feel like you want to go with your variant, I won't stand in the way, but I
> > also wouldn't give it my ack or alike (which you don't depend on anyway).
> 
> After careful consideration, I think I'll submit my version of the
> patch, assuming that Anthony is OK with it. (FYI I had to keep your
> signed-off-by line for copyright reasons: you own the copyright on some
> of the changes.)
> 
> Anthony, what do you think of the following:

I do prefare the version without cast, but this is fine too. It is much
easier to understand that nothing changes beside the fact that field are
filled directly on the shared ring rather than from the stack. So I'm
fine with it.

You can add my
Acked-by: Anthony PERARD <anthony.perard@citrix.com>

> ---
> 
> xen/disk: don't leak stack data via response ring
> 
> Rather than constructing a local structure instance on the stack, fill
> the fields directly on the shared ring, just like other (Linux)
> backends do. Build on the fact that all response structure flavors are
> actually identical (aside from alignment and padding at the end).
> 
> This is XSA-216.
> 
> Reported by: Anthony Perard <anthony.perard@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 3a22805..9200511 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -769,31 +769,30 @@ static int blk_send_response_one(struct ioreq *ioreq)
>      struct XenBlkDev  *blkdev = ioreq->blkdev;
>      int               send_notify   = 0;
>      int               have_requests = 0;
> -    blkif_response_t  resp;
> -    void              *dst;
> -
> -    resp.id        = ioreq->req.id;
> -    resp.operation = ioreq->req.operation;
> -    resp.status    = ioreq->status;
> +    blkif_response_t  *resp;
>  
>      /* Place on the response ring for the relevant domain. */
>      switch (blkdev->protocol) {
>      case BLKIF_PROTOCOL_NATIVE:
> -        dst = RING_GET_RESPONSE(&blkdev->rings.native, blkdev->rings.native.rsp_prod_pvt);
> +        resp = (blkif_response_t *) RING_GET_RESPONSE(&blkdev->rings.native,
> +                                 blkdev->rings.native.rsp_prod_pvt);
>          break;
>      case BLKIF_PROTOCOL_X86_32:
> -        dst = RING_GET_RESPONSE(&blkdev->rings.x86_32_part,
> -                                blkdev->rings.x86_32_part.rsp_prod_pvt);
> +        resp = (blkif_response_t *) RING_GET_RESPONSE(&blkdev->rings.x86_32_part,
> +                                 blkdev->rings.x86_32_part.rsp_prod_pvt);
>          break;
>      case BLKIF_PROTOCOL_X86_64:
> -        dst = RING_GET_RESPONSE(&blkdev->rings.x86_64_part,
> -                                blkdev->rings.x86_64_part.rsp_prod_pvt);
> +        resp = (blkif_response_t *) RING_GET_RESPONSE(&blkdev->rings.x86_64_part,
> +                                 blkdev->rings.x86_64_part.rsp_prod_pvt);
>          break;
>      default:
> -        dst = NULL;
>          return 0;
>      }
> -    memcpy(dst, &resp, sizeof(resp));
> +
> +    resp->id        = ioreq->req.id;
> +    resp->operation = ioreq->req.operation;
> +    resp->status    = ioreq->status;
> +
>      blkdev->rings.common.rsp_prod_pvt++;
>  
>      RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&blkdev->rings.common, send_notify);

-- 
Anthony PERARD

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