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

[Stefan Bader]

On 30.09.2017 17:56, 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
> 
> (backported from commit 089bc0143f489bd3a4578bdff5f4ca68fb26f341)
> Signed-off-by: Shrirang Bagul <shrirang.bagul at canonical.com>
> ---
>  drivers/block/xen-blkback/blkback.c | 28 +++++++++++++++-------------
>  drivers/block/xen-blkback/common.h  | 25 +++++--------------------
>  2 files changed, 20 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
> index 33e23a7a691f..903227c11e77 100644
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -1407,33 +1407,35 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
>  static void make_response(struct xen_blkif *blkif, u64 id,
>  			  unsigned short op, int st)
>  {
> -	struct blkif_response  resp;
> +	struct blkif_response *resp;
>  	unsigned long     flags;
> -	union blkif_back_rings *blk_rings = &blkif->blk_rings;
> +	union blkif_back_rings *blk_rings;
>  	int notify;
>  
> -	resp.id        = id;
> -	resp.operation = op;
> -	resp.status    = st;
> -
> -	spin_lock_irqsave(&blkif->blk_ring_lock, flags);
> +	spin_unlock_irqrestore(&blkif->blk_ring_lock, flags);

Why changing the locking ^here? There still is an unlock below and the original
patch did not touch this either.


> +	blk_rings = &blkif->blk_rings;
>  	/* Place on the response ring for the relevant domain. */
>  	switch (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(&blkif->blk_ring_lock, flags);
> diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
> index c929ae22764c..04cfee719334 100644
> --- a/drivers/block/xen-blkback/common.h
> +++ b/drivers/block/xen-blkback/common.h
> @@ -74,9 +74,8 @@ extern unsigned int xen_blkif_max_ring_order;
>  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                   */
> @@ -128,14 +127,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 {
> @@ -192,18 +183,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;
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20171005/8de8f099/attachment.sig>