Skip to contentSkip to conversation

Prevent the ipmi_bt_sm driver from attempting to send larger messages than hardware accept

PATCH In review Dmitry Rakhchev 2 participants 3 messages Nov 11, 2014 – Nov 14, 2014 IPMI

lore
Share & tools
lore

Dmitry Rakhchev

author
#0 lore raw

PATCH 1/1 Prevent the ipmi_bt_sm driver from attempting to send larger messages than hardware accept

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
Quoted 1 line

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 <[email protected]>
---
 drivers/char/ipmi/ipmi_bt_sm.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/char/ipmi/ipmi_bt_sm.c +5 -1
index 61e7161..c64ad02 100644
@@ -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
Signature · 1 line
1.8.4
#1 lore raw

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.

Rocky, what do you think?

-corey

Quoted 87 lines · Dmitry Rakhchev

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 <[email protected]>
---
 drivers/char/ipmi/ipmi_bt_sm.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/char/ipmi/ipmi_bt_sm.c +5 -1
index 61e7161..c64ad02 100644
@@ -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
author
#2 lore raw

Hello Corey,

Quoted 5 lines · Corey Minyard

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

Quoted 63 lines

Rocky, what do you think?

-corey

Quoted 57 lines · Dmitry Rakhchev

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 <[email protected]>

---

 drivers/char/ipmi/ipmi_bt_sm.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
(quoted diff) +0 -0

b/drivers/char/ipmi/ipmi_bt_sm.c index 61e7161..c64ad02 100644

drivers/char/ipmi/ipmi_bt_sm.c +0 -0
@@ -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 */

Quoted 1 line
Quoted 1 line

+#define BT_MINIMAL_MESSAGE_SIZE 63 /* Does not include the length

byte */

Quoted 57 lines
Quoted 57 lines

/*

* States are written in chronological order and usually cover

drivers/char/ipmi/ipmi_bt_sm.c +1 -0
@@ -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;
drivers/char/ipmi/ipmi_bt_sm.c +1 -0
 	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;
drivers/char/ipmi/ipmi_bt_sm.c +1 -1
 	/* 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)
drivers/char/ipmi/ipmi_bt_sm.c +1 -0
 		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];
drivers/char/ipmi/ipmi_bt_sm.c +0 -0
 			bt->BT_CAP_req2rsp = BT_CAP[6] * USEC_PER_SEC;
 			bt->BT_CAP_retries = BT_CAP[7];
 
 		} else