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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 26 Jun 2017, Jan Beulich wrote:
> >>> Stefano Stabellini <sstabellini@xxxxxxxxxx> 06/23/17 8:43 PM >>>
> >On Fri, 23 Jun 2017, Jan Beulich wrote:
> >> >>> On 22.06.17 at 20:52, <sstabellini@xxxxxxxxxx> 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@xxxxxxxxxx>
Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
Signed-off-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>

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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel