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

[Shrirang Bagul]

On Thu, 2017-10-05 at 08:43 +0200, Stefan Bader wrote:
> 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.
Arrgh! A silly mistake. Will send v2 to correct this for Xenial/Trusty.

- Shrirang
> 
> 
> > +	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: 801 bytes
Desc: This is a digitally signed message part
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20171005/308b0050/attachment.sig>