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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 21 Jun 2017, Jan Beulich wrote:
> >>> On 20.06.17 at 23:48, <sstabellini@xxxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel