[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] xen/disk: don't leak stack data via response ri
From: |
Stefano Stabellini |
Subject: |
Re: [Qemu-devel] [PATCH] xen/disk: don't leak stack data via response ring |
Date: |
Tue, 20 Jun 2017 14:48:07 -0700 (PDT) |
User-agent: |
Alpine 2.10 (DEB 1266 2009-07-14) |
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 <address@hidden>
> Signed-off-by: Jan Beulich <address@hidden>
> Reviewed-by: Konrad Rzeszutek Wilk <address@hidden>
> Acked-by: Anthony PERARD <address@hidden>
> ---
> 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);
>
>
>