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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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).

Jan


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