NACK: [Z SRU][PATCH] xen-blkback: don't leak stack data via response ring

[Shrirang Bagul]

On Sat, 2017-09-30 at 23:56 +0800, Shrirang Bagul wrote:
> From: Jan Beulich <jbeulich at suse.com>
> 
> Rather than constructing a local structure instance on the stack, fill
> the fields directly on the shared ring, just like other 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.
> 
> Cc: stable at vger.kernel.org
> 
> Signed-off-by: Jan Beulich <jbeulich at suse.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk at oracle.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk at oracle.com>
> 
> This fixes CVE-2017-10911
> 
> (cherry picked from commit 089bc0143f489bd3a4578bdff5f4ca68fb26f341)
> Signed-off-by: Shrirang Bagul <shrirang.bagul at canonical.com>
Will resend with updated patch for Xenial/Trusty.
> ---
>  drivers/block/xen-blkback/blkback.c | 23 ++++++++++++-----------
>  drivers/block/xen-blkback/common.h  | 25 +++++--------------------
>  2 files changed, 17 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-
> blkback/blkback.c
> index 726c32e35db9..fbe0dfdffc0d 100644
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -1436,34 +1436,35 @@ static int dispatch_rw_block_io(struct xen_blkif_ring
> *ring,
>  static void make_response(struct xen_blkif_ring *ring, u64 id,
>  			  unsigned short op, int st)
>  {
> -	struct blkif_response  resp;
> +	struct blkif_response *resp;
>  	unsigned long     flags;
>  	union blkif_back_rings *blk_rings;
>  	int notify;
>  
> -	resp.id        = id;
> -	resp.operation = op;
> -	resp.status    = st;
> -
>  	spin_lock_irqsave(&ring->blk_ring_lock, flags);
>  	blk_rings = &ring->blk_rings;
>  	/* Place on the response ring for the relevant domain. */
>  	switch (ring->blkif->blk_protocol) {
>  	case BLKIF_PROTOCOL_NATIVE:
> -		memcpy(RING_GET_RESPONSE(&blk_rings->native, blk_rings-
> >native.rsp_prod_pvt),
> -		       &resp, sizeof(resp));
> +		resp = RING_GET_RESPONSE(&blk_rings->native,
> +					 blk_rings->native.rsp_prod_pvt);
>  		break;
>  	case BLKIF_PROTOCOL_X86_32:
> -		memcpy(RING_GET_RESPONSE(&blk_rings->x86_32, blk_rings-
> >x86_32.rsp_prod_pvt),
> -		       &resp, sizeof(resp));
> +		resp = RING_GET_RESPONSE(&blk_rings->x86_32,
> +					 blk_rings->x86_32.rsp_prod_pvt);
>  		break;
>  	case BLKIF_PROTOCOL_X86_64:
> -		memcpy(RING_GET_RESPONSE(&blk_rings->x86_64, blk_rings-
> >x86_64.rsp_prod_pvt),
> -		       &resp, sizeof(resp));
> +		resp = RING_GET_RESPONSE(&blk_rings->x86_64,
> +					 blk_rings->x86_64.rsp_prod_pvt);
>  		break;
>  	default:
>  		BUG();
>  	}
> +
> +	resp->id        = id;
> +	resp->operation = op;
> +	resp->status    = st;
> +
>  	blk_rings->common.rsp_prod_pvt++;
>  	RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&blk_rings->common, notify);
>  	spin_unlock_irqrestore(&ring->blk_ring_lock, flags);
> diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-
> blkback/common.h
> index dea61f6ab8cb..aca9397277e9 100644
> --- a/drivers/block/xen-blkback/common.h
> +++ b/drivers/block/xen-blkback/common.h
> @@ -75,9 +75,8 @@ extern unsigned int xenblk_max_queues;
>  struct blkif_common_request {
>  	char dummy;
>  };
> -struct blkif_common_response {
> -	char dummy;
> -};
> +
> +/* i386 protocol version */
>  
>  struct blkif_x86_32_request_rw {
>  	uint8_t        nr_segments;  /* number of segments                   */
> @@ -129,14 +128,6 @@ struct blkif_x86_32_request {
>  	} u;
>  } __attribute__((__packed__));
>  
> -/* i386 protocol version */
> -#pragma pack(push, 4)
> -struct blkif_x86_32_response {
> -	uint64_t        id;              /* copied from request */
> -	uint8_t         operation;       /* copied from request */
> -	int16_t         status;          /* BLKIF_RSP_???       */
> -};
> -#pragma pack(pop)
>  /* x86_64 protocol version */
>  
>  struct blkif_x86_64_request_rw {
> @@ -193,18 +184,12 @@ struct blkif_x86_64_request {
>  	} u;
>  } __attribute__((__packed__));
>  
> -struct blkif_x86_64_response {
> -	uint64_t       __attribute__((__aligned__(8))) id;
> -	uint8_t         operation;       /* copied from request */
> -	int16_t         status;          /* BLKIF_RSP_???       */
> -};
> -
>  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 __packed);
>  DEFINE_RING_TYPES(blkif_x86_64, struct blkif_x86_64_request,
> -		  struct blkif_x86_64_response);
> +		  struct blkif_response);
>  
>  union blkif_back_rings {
>  	struct blkif_back_ring        native;
> -- 
> 2.11.0
> 
>