[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: |
Wed, 21 Jun 2017 11:46:08 -0700 (PDT) |
User-agent: |
Alpine 2.10 (DEB 1266 2009-07-14) |
On Wed, 21 Jun 2017, Jan Beulich wrote:
> >>> On 20.06.17 at 23:48, <address@hidden> 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.
- [Qemu-devel] [PATCH v2 00/21] travis: speedup to reduce failures, Philippe Mathieu-Daudé, 2017/06/21
- [Qemu-devel] [PATCH v2 01/21] tests: add missing dependency to build QTEST_QEMU_BINARY, Philippe Mathieu-Daudé, 2017/06/21
- [Qemu-devel] [PATCH v2 02/21] travis: retry if llvm.org timeouts, Philippe Mathieu-Daudé, 2017/06/21
- [Qemu-devel] [PATCH v2 03/21] travis: install more library dependencies, Philippe Mathieu-Daudé, 2017/06/21
- [Qemu-devel] [PATCH v2 04/21] travis: install more library dependencies, Philippe Mathieu-Daudé, 2017/06/21
- [Qemu-devel] [PATCH v2 05/21] scripts/run-coverity-scan: Script to run Coverity Scan build, Philippe Mathieu-Daudé, 2017/06/21
- [Qemu-devel] [PATCH v2 07/21] travis: update sudo-enabled Trusty images, Philippe Mathieu-Daudé, 2017/06/21
- [Qemu-devel] [PATCH v2 06/21] travis: Add config to do a Coverity Scan upload, Philippe Mathieu-Daudé, 2017/06/21
- [Qemu-devel] [PATCH v2 08/21] travis: use gcc-6 sanitizers, Philippe Mathieu-Daudé, 2017/06/21
- [Qemu-devel] [PATCH v2 09/21] travis: enable multiple caching features, Philippe Mathieu-Daudé, 2017/06/21
- [Qemu-devel] [PATCH v2 10/21] travis: increase S3 cache timeout, Philippe Mathieu-Daudé, 2017/06/21