Date:	Fri, 14 Nov 2014 14:45:40 +0300
From:	Dmitry Rakhchev <rda@...eonpoint.com>
To:	minyard@....org
Cc:	openipmi-developer@...ts.sourceforge.net,
	linux-kernel@...r.kernel.org, Rocky Craig <rocky.craig@...com>
Subject: Re: [PATCH 1/1] Prevent the ipmi_bt_sm driver from attempting to send larger messages than hardware accept

Hello Corey, 

On Wednesday, November 12, 2014 09:05:00 AM Corey Minyard wrote:
> This is probably a good idea, but I would expect that the BMC would
> respond with a "Request data field length limit exceeded. (C8h)"
> completion code in this case instead of being silent.  Though as far as
> I can tell there's nothing in the spec that says what should happen in
> this case.
The problem I'm trying to solve is that the message is corrupted in the 
hardware mailbox due to address wrap _before_ it can be read by BMC. And BMC 
reads corrupted message (with the header destroyed first), so sometimes 
there's a reply (mostly with 0xC1 completion code), but it cannot be matched 
to request since command and sequence fields were corrupted and do not match 
the message the driver believes it has send.

What make things worse, if only the length is overwritten in the message box, 
the message can be truncated. If the length is greater than the header
BMC can even execute it successfully and return valid response, but e.g. write 
less data than the requester expects. 

If the driver also overwrite the command field, than some other command can be 
executed instead.

Regards,
Dmitry
> 
> Rocky, what do you think?
> 
> -corey
> 
> On 11/11/2014 04:40 AM, Dmitry Rakhchev wrote:
> > While testing our IPMC implementation for maximal message size I've
> > encountered hardware buffer overflow in BT driver.
> > 
> > The problem is visible as a timeout on the driver side, and our IPMC
> > received garbage instead of message.
> > 
> > To reproduce:
> > # ipmitool raw 0xa 0x12 0x0 0x0 0x0   0x01 0x00 0x00 0x01 0x08 0x10 0x00
> > 0xe6 0x01 0x07 0x19 0x00 0x26 0x70 0xc3 0x50 0x50 0x53 0xd0 0x42 0x4d
> > 0x52 0x2d 0x41 0x32 0x46 0x2d 0x41 0x54 0x43 0x41 0x2d 0x42 0x54 0x52
> > 0xca 0x50 0x50 0x53 0x78 0x78 0x78 0x78 0x78 0x78 0x78 0xc2 0x41 0x20
> > 0xcc 0x66 0x72 0x75 0x2d 0x69 0x6e 0x66 0x01 Unable to send RAW command
> > (channel=0x0 netfn=0xa lun=0x0 cmd=0x12 rsp=0xc3): Timeout
> > 
> > From dmesg:
> > [  353.844011] IPMI BT: timeout in RD_WAIT [ ] 1 retries left
> > [  355.845011] IPMI BT: timeout in RD_WAIT [ ]
> > [  355.845017] failed 2 retries, sending error response
> > 
> > Dump of messages on IPMC:
> > <B>: <-- { 18 35 01 }
> > <B>: --> { 1C 35 01 00 12 80 01 20 02 2D 0A 40 00 DA BB }
> > <B>: <-- { B0 36 00 00 }
> > <B>: --> { B4 36 00 00 00 32 01 00 }
> > <B>: <-- { B0 37 01 00 }
> > <B>: --> { B4 37 01 00 00 42 84 FF 00 00 00 }
> > <B>: <-- { 28 }
> > <B>: <-- { 28 }
> > 
> > Last two messages are the corrupted raw message from ipmitool above (extra
> > 0x01 byte overwrite the length).
> > 
> > Maximum message size check in bt_start_transaction shall respect maximum
> > request size reported by Get BT Capabilities.
> > 
> > Solution: store the size returned from IPMC and use it to check maximal
> > allowed message size. Use minimal value 63 required by IPMI until the real
> > maximum is known.
> > 
> > Changes tested on 3.15.10-200.fc20.i686+PAE.
> > 
> > Signed-off-by: Dmitry Rakhchev <rda@...eonpoint.com>
> > ---
> > 
> >  drivers/char/ipmi/ipmi_bt_sm.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/char/ipmi/ipmi_bt_sm.c
> > b/drivers/char/ipmi/ipmi_bt_sm.c index 61e7161..c64ad02 100644
> > --- a/drivers/char/ipmi/ipmi_bt_sm.c
> > +++ b/drivers/char/ipmi/ipmi_bt_sm.c
> > @@ -58,6 +58,7 @@ MODULE_PARM_DESC(bt_debug, "debug bitmask, 1=enable,
> > 2=messages, 4=states");> 
> >  #define BT_NORMAL_TIMEOUT	5	/* seconds */
> >  #define BT_NORMAL_RETRY_LIMIT	2
> >  #define BT_RESET_DELAY		6	/* seconds after warm reset */
> > 
> > +#define BT_MINIMAL_MESSAGE_SIZE	63	/* Does not include the length 
byte */
> > 
> >  /*
> >  
> >   * States are written in chronological order and usually cover
> > 
> > @@ -105,6 +106,7 @@ struct si_sm_data {
> > 
> >  	int		nonzero_status;	/* hung BMCs stay all 0 */
> >  	enum bt_states	complete;	/* to divert the state machine */
> >  	int		BT_CAP_outreqs;
> > 
> > +	int		BT_CAP_inbufsz;
> > 
> >  	long		BT_CAP_req2rsp;
> >  	int		BT_CAP_retries;	/* Recommended retries */
> >  
> >  };
> > 
> > @@ -203,6 +205,7 @@ static unsigned int bt_init_data(struct si_sm_data
> > *bt, struct si_sm_io *io)> 
> >  	bt->complete = BT_STATE_IDLE;	/* end here */
> >  	bt->BT_CAP_req2rsp = BT_NORMAL_TIMEOUT * USEC_PER_SEC;
> >  	bt->BT_CAP_retries = BT_NORMAL_RETRY_LIMIT;
> > 
> > +	bt->BT_CAP_inbufsz = BT_MINIMAL_MESSAGE_SIZE;
> > 
> >  	/* BT_CAP_outreqs == zero is a flag to read BT Capabilities */
> >  	return 3; /* We claim 3 bytes of space; ought to check SPMI table */
> >  
> >  }
> > 
> > @@ -229,7 +232,7 @@ static int bt_start_transaction(struct si_sm_data *bt,
> > 
> >  	if (size < 2)
> >  	
> >  		return IPMI_REQ_LEN_INVALID_ERR;
> > 
> > -	if (size > IPMI_MAX_MSG_LENGTH)
> > +	if ((size + 1) > bt->BT_CAP_inbufsz)
> > 
> >  		return IPMI_REQ_LEN_EXCEEDED_ERR;
> >  	
> >  	if (bt->state == BT_STATE_LONG_BUSY)
> > 
> > @@ -651,6 +654,7 @@ static enum si_sm_result bt_event(struct si_sm_data
> > *bt, long time)> 
> >  		bt_init_data(bt, bt->io);
> >  		if ((i == 8) && !BT_CAP[2]) {
> >  		
> >  			bt->BT_CAP_outreqs = BT_CAP[3];
> > 
> > +			bt->BT_CAP_inbufsz = BT_CAP[4];
> > 
> >  			bt->BT_CAP_req2rsp = BT_CAP[6] * USEC_PER_SEC;
> >  			bt->BT_CAP_retries = BT_CAP[7];
> >  		
> >  		} else
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists