IB/mad: In validate_mad, validate CM method and attribute
PATCH 1/1 IB/mad: In validate_mad, validate CM method and attribute
Receipt of CM MAD with response method for other than ClassPortInfo attribute is invalid.
CM attributes other than ClassPortInfo use send method only and GetResp is valid for ClassPortInfo attribute. Note also that the CM ClassPortInfo is not currently supported.
The SRP initiator does not maintain a timeout policy for CM connect requests relies on the CM layer to do that. The result was that the SRP initiator hung as the connect request never completed.
A new SRP target has been observed to respond to Send CM REQ with GetResp of CM REQ with bad status. This is non conformant with IBA spec but exposes a vulnerability in the current MAD/CM code which will respond to the incoming GetResp of CM REQ as if it was a valid incoming Send of CM REQ rather than tossing this on the floor. It also causes the MAD layer not to retransmit the original REQ even though it has not received a REP.
--- drivers/infiniband/core/mad.c | 8 ++++++++ include/rdma/ib_mad.h | 2 ++ 2 files changed, 10 insertions(+), 0 deletions(-)
drivers/infiniband/core/mad.c +8 -0
@@ -1811,6 +1811,14 @@ static int validate_mad(const struct ib_mad_hdr *mad_hdr,if (qp_num ==0 ) valid =1 ; }else { +/* CM attributes other than ClassPortInfo only use Send method */ +if (mad_hdr->mgmt_class ==IB_MGMT_CLASS_CM ) { +if (mad_hdr->attr_id !=IB_MGMT_CLASSPORTINFO_ATTR_ID ) { +if (mad_hdr->method !=IB_MGMT_METHOD_SEND ) + goto out; + }else if (mad_hdr->method !=IB_MGMT_METHOD_GET_RESP ) + goto out; + }/* Filter GSI packets sent to QP0 */ if (qp_num !=0 ) valid =1 ;
include/rdma/ib_mad.h +2 -0
@@ -237,6 +237,8 @@ struct ib_vendor_mad {u8 data[IB_MGMT_VENDOR_DATA ]; }; +#define IB_MGMT_CLASSPORTINFO_ATTR_ID cpu_to_be16(0x0001) +struct ib_class_port_info {u8 base_version;u8 class_version;
Signature · 6 lines
1.7.8.2 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 56447CA2.1070802-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org
PiBSZWNlaXB0IG9mIENNIE1BRCB3aXRoIHJlc3BvbnNlIG1ldGhvZCBmb3Igb3RoZXIgdGhhbiBD bGFzc1BvcnRJbmZvDQo+IGF0dHJpYnV0ZSBpcyBpbnZhbGlkLg0KPiANCj4gQ00gYXR0cmlidXRl cyBvdGhlciB0aGFuIENsYXNzUG9ydEluZm8gdXNlIHNlbmQgbWV0aG9kIG9ubHkNCj4gYW5kIEdl dFJlc3AgaXMgdmFsaWQgZm9yIENsYXNzUG9ydEluZm8gYXR0cmlidXRlLg0KPiBOb3RlIGFsc28g dGhhdCB0aGUgQ00gQ2xhc3NQb3J0SW5mbyBpcyBub3QgY3VycmVudGx5IHN1cHBvcnRlZC4NCj4g DQo+IFRoZSBTUlAgaW5pdGlhdG9yIGRvZXMgbm90IG1haW50YWluIGEgdGltZW91dCBwb2xpY3kg Zm9yIENNIGNvbm5lY3QNCj4gcmVxdWVzdHMgcmVsaWVzIG9uIHRoZSBDTSBsYXllciB0byBkbyB0 aGF0LiBUaGUgcmVzdWx0IHdhcyB0aGF0DQo+IHRoZSBTUlAgaW5pdGlhdG9yIGh1bmcgYXMgdGhl IGNvbm5lY3QgcmVxdWVzdCBuZXZlciBjb21wbGV0ZWQuDQo+IA0KPiBBIG5ldyBTUlAgdGFyZ2V0 IGhhcyBiZWVuIG9ic2VydmVkIHRvIHJlc3BvbmQgdG8gU2VuZCBDTSBSRVENCj4gd2l0aCBHZXRS ZXNwIG9mIENNIFJFUSB3aXRoIGJhZCBzdGF0dXMuIFRoaXMgaXMgbm9uIGNvbmZvcm1hbnQNCj4g d2l0aCBJQkEgc3BlYyBidXQgZXhwb3NlcyBhIHZ1bG5lcmFiaWxpdHkgaW4gdGhlIGN1cnJlbnQg TUFEL0NNDQo+IGNvZGUgd2hpY2ggd2lsbCByZXNwb25kIHRvIHRoZSBpbmNvbWluZyBHZXRSZXNw IG9mIENNIFJFUSBhcyBpZg0KPiBpdCB3YXMgYSB2YWxpZCBpbmNvbWluZyBTZW5kIG9mIENNIFJF USByYXRoZXIgdGhhbiB0b3NzaW5nDQo+IHRoaXMgb24gdGhlIGZsb29yLiBJdCBhbHNvIGNhdXNl cyB0aGUgTUFEIGxheWVyIG5vdCB0bw0KPiByZXRyYW5zbWl0IHRoZSBvcmlnaW5hbCBSRVEgZXZl biB0aG91Z2ggaXQgaGFzIG5vdCByZWNlaXZlZCBhIFJFUC4NCj4gDQo+IFJldmlld2VkLWJ5OiBT YWdpIEdyaW1iZXJnIDxzYWdpZ0BtZWxsYW5veC5jb20+DQo+IFNpZ25lZC1vZmYtYnk6IEhhbCBS b3NlbnN0b2NrIDxoYWxAbWVsbGFub3guY29tPg0KDQpSZXZpZXdlZC1ieTogU2VhbiBIZWZ0eSA8 c2Vhbi5oZWZ0eUBpbnRlbC5jb20+DQo=
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 7 lines · Hal Rosenstock
A new SRP target has been observed to respond to Send CM REQ with GetResp of CM REQ with bad status. This is non conformant with IBA spec but exposes a vulnerability in the current MAD/CM code which will respond to the incoming GetResp of CM REQ as if it was a valid incoming Send of CM REQ rather than tossing this on the floor. It also causes the MAD layer not to retransmit the original REQ even though it has not received a REP.
Hello Hal,
With which SRP target has this behavior been observed ? Has this patch been tested with the LIO SRP target ?
Thanks,
Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 5 lines
Hello Hal,
With which SRP target has this behavior been observed ? Has this patch been tested with the LIO SRP target ?
Hi Bart,
This issue was detected when testing a new array with SRP support. This does not involve LIO as the Linux CM stack does not behave in the way described in this patch.
Sagi.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 33 lines · Hal Rosenstock
Receipt of CM MAD with response method for other than ClassPortInfo attribute is invalid.
CM attributes other than ClassPortInfo use send method only and GetResp is valid for ClassPortInfo attribute. Note also that the CM ClassPortInfo is not currently supported.
The SRP initiator does not maintain a timeout policy for CM connect requests relies on the CM layer to do that. The result was that the SRP initiator hung as the connect request never completed.
A new SRP target has been observed to respond to Send CM REQ with GetResp of CM REQ with bad status. This is non conformant with IBA spec but exposes a vulnerability in the current MAD/CM code which will respond to the incoming GetResp of CM REQ as if it was a valid incoming Send of CM REQ rather than tossing this on the floor. It also causes the MAD layer not to retransmit the original REQ even though it has not received a REP.
--- drivers/infiniband/core/mad.c | 8 ++++++++ include/rdma/ib_mad.h | 2 ++ 2 files changed, 10 insertions(+), 0 deletions(-)drivers/infiniband/core/mad.c +0
-0@@ -1811,6 +1811,14 @@ static int validate_mad(const struct ib_mad_hdr *mad_hdr,if (qp_num ==0 )
drivers/infiniband/core/mad.c static int validate_mad(const struct ib_mad_hdr *mad_hdr, · line 1821
Quoted 10 lines
drivers/infiniband/core/mad.c +8
-0valid =1 ; }else { +/* CM attributes other than ClassPortInfo only use Send method */ +if (mad_hdr->mgmt_class ==IB_MGMT_CLASS_CM ) { +if (mad_hdr->attr_id !=IB_MGMT_CLASSPORTINFO_ATTR_ID ) { +if (mad_hdr->method !=IB_MGMT_METHOD_SEND ) + goto out; + }else if (mad_hdr->method !=IB_MGMT_METHOD_GET_RESP ) + goto out; + }
Doesn't this invalidate a CM Get(ClassPortInfo) mad? Is that the intention because it is not supported in the CM?
For the future it seems like these types of checks should be done at the class level. But I'm not advocating that right now.
Ira
Quoted 23 lines
drivers/infiniband/core/mad.c +0
-0/* Filter GSI packets sent to QP0 */ if (qp_num !=0 ) valid =1 ;include/rdma/ib_mad.h +2
-0@@ -237,6 +237,8 @@ struct ib_vendor_mad {u8 data[IB_MGMT_VENDOR_DATA ]; }; +#define IB_MGMT_CLASSPORTINFO_ATTR_ID cpu_to_be16(0x0001) +struct ib_class_port_info {u8 base_version;u8 class_version;Signature · 6 lines
1.7.8.2 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 20151112181339.GA16488-W4f6Xiosr+yv7QzWx2u06xL4W9x8LtSr@public.gmane.org
Quoted 11 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ /* CM attributes other than ClassPortInfo only use Send method*/
drivers/infiniband/core/mad.c static int validate_mad(const struct ib_mad_hdr *mad_hdr, · line 1821
Quoted 7 lines
drivers/infiniband/core/mad.c +7
-0+if (mad_hdr->mgmt_class ==IB_MGMT_CLASS_CM ) { +if (mad_hdr->attr_id !=IB_MGMT_CLASSPORTINFO_ATTR_ID ) { +if (mad_hdr->method !=IB_MGMT_METHOD_SEND ) + goto out; + }else if (mad_hdr->method !=IB_MGMT_METHOD_GET_RESP ) + goto out; + }Doesn't this invalidate a CM Get(ClassPortInfo) mad?
I believe this does. I think you could remove the else if clause and let the received MAD get passed to the CM. It would be dropped there as unsupported. The net result is likely the same. --
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 1828884A29C6694DAF28B7E6B8A82373A9BACD7D-8oqHQFITsIHTXloPLtfHfbfspsVTdybXVpNB7YpNyf8@public.gmane.org
Quoted 12 lines · Sean
Quoted 11 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ /* CM attributes other than ClassPortInfo only use Send method*/
drivers/infiniband/core/mad.c static int validate_mad(const struct ib_mad_hdr *mad_hdr, · line 1821
Quoted 7 lines
drivers/infiniband/core/mad.c +7
-0+if (mad_hdr->mgmt_class ==IB_MGMT_CLASS_CM ) { +if (mad_hdr->attr_id !=IB_MGMT_CLASSPORTINFO_ATTR_ID ) { +if (mad_hdr->method !=IB_MGMT_METHOD_SEND ) + goto out; + }else if (mad_hdr->method !=IB_MGMT_METHOD_GET_RESP ) + goto out; + }Doesn't this invalidate a CM Get(ClassPortInfo) mad?
Quoted 3 lines
I believe this does. I think you could remove the else if clause and let the received MAD get passed to the CM. It would be dropped there as unsupported. The net result is likely the same.
IIRC responding to Get(CPI) is mandatory?
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 20151112221300.GA12250-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org
Quoted 1 line
IIRC responding to Get(CPI) is mandatory?
Maybe the drivers are responding? I don't believe that the CM does. --
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 1828884A29C6694DAF28B7E6B8A82373A9BACE42-8oqHQFITsIHTXloPLtfHfbfspsVTdybXVpNB7YpNyf8@public.gmane.org
Quoted 3 lines · Sean
Quoted 1 line
IIRC responding to Get(CPI) is mandatory?
Maybe the drivers are responding? I don't believe that the CM does.
Sorry for the confusion. linux kernel CM does not respond to Get(CPI). I was just trying to add the validation code in for that since it was being added for the remainder of the CM attributes even though CPI is not currently supported there.
-- Hal
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 13 lines · Jason Gunthorpe
Quoted 12 lines · Sean
Quoted 11 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ /* CM attributes other than ClassPortInfo only use Send method*/
Review
Quoted 7 lines
(quoted diff) +7
-0+ if (mad_hdr->mgmt_class == IB_MGMT_CLASS_CM) { + if (mad_hdr->attr_id != IB_MGMT_CLASSPORTINFO_ATTR_ID) { + if (mad_hdr->method != IB_MGMT_METHOD_SEND) + goto out; + } else if (mad_hdr->method != IB_MGMT_METHOD_GET_RESP) + goto out; + }Doesn't this invalidate a CM Get(ClassPortInfo) mad?
Quoted 1 line
Quoted 1 line
I believe this does.
My bad. That wasn't my intention.
Quoted 3 lines
Quoted 3 lines
I think you could remove the else if clause
and let the received MAD get passed to the CM. It would be dropped there as unsupported. The net result is likely the same.
I'm dropping else clause for CM CPI validation in next version of this patch.
Quoted 1 line
IIRC responding to Get(CPI) is mandatory?
Yes it's mandatory to support CPI (and get method of CPI) on a class. However, it's not currently done in CM.
-- Hal
Quoted 2 lines
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 12 lines · Sean
Quoted 11 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ /* CM attributes other than ClassPortInfo only use Send method*/
drivers/infiniband/core/mad.c static int validate_mad(const struct ib_mad_hdr *mad_hdr, · line 1821
Quoted 7 lines
drivers/infiniband/core/mad.c +7
-0+if (mad_hdr->mgmt_class ==IB_MGMT_CLASS_CM ) { +if (mad_hdr->attr_id !=IB_MGMT_CLASSPORTINFO_ATTR_ID ) { +if (mad_hdr->method !=IB_MGMT_METHOD_SEND ) + goto out; + }else if (mad_hdr->method !=IB_MGMT_METHOD_GET_RESP ) + goto out; + }Doesn't this invalidate a CM Get(ClassPortInfo) mad?
Quoted 1 line
I believe this does. I think you could remove the else if clause and let the received MAD get passed to the CM. It would be dropped there as unsupported. The net result is likely the same.
Right now this is indeed the case as CPI is not supported in CM. --
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 39 lines · ira.weiny
Quoted 38 lines · Hal Rosenstock
Receipt of CM MAD with response method for other than ClassPortInfo attribute is invalid.
CM attributes other than ClassPortInfo use send method only and GetResp is valid for ClassPortInfo attribute. Note also that the CM ClassPortInfo is not currently supported.
The SRP initiator does not maintain a timeout policy for CM connect requests relies on the CM layer to do that. The result was that the SRP initiator hung as the connect request never completed.
A new SRP target has been observed to respond to Send CM REQ with GetResp of CM REQ with bad status. This is non conformant with IBA spec but exposes a vulnerability in the current MAD/CM code which will respond to the incoming GetResp of CM REQ as if it was a valid incoming Send of CM REQ rather than tossing this on the floor. It also causes the MAD layer not to retransmit the original REQ even though it has not received a REP.
--- drivers/infiniband/core/mad.c | 8 ++++++++ include/rdma/ib_mad.h | 2 ++ 2 files changed, 10 insertions(+), 0 deletions(-)drivers/infiniband/core/mad.c +3
-0@@ -1811,6 +1811,14 @@ static int validate_mad(const struct ib_mad_hdr *mad_hdr,if (qp_num ==0 ) valid =1 ; }else { +/* CM attributes other than ClassPortInfo only use Send method */ +if (mad_hdr->mgmt_class ==IB_MGMT_CLASS_CM ) { +if (mad_hdr->attr_id !=IB_MGMT_CLASSPORTINFO_ATTR_ID ) {
Quoted 8 lines
drivers/infiniband/core/mad.c static int validate_mad(const struct ib_mad_hdr *mad_hdr, · line 1821
Quoted 5 lines
drivers/infiniband/core/mad.c +5
-0+if (mad_hdr->method !=IB_MGMT_METHOD_SEND ) + goto out; + }else if (mad_hdr->method !=IB_MGMT_METHOD_GET_RESP ) + goto out; + }Doesn't this invalidate a CM Get(ClassPortInfo) mad? Is that the intention because it is not supported in the CM?
The intention was to future proof it for when it would be supported but I misexecuted this as it does not currently handle get as you pointed out. v2 of patch to follow...
Quoted 2 lines
For the future it seems like these types of checks should be done at the class level. But I'm not advocating that right now.
Yes, architecturally these sort of checks belong at the class level. I started with the checks at the class level (in CM) but since CM relies on the MAD retransmission mechanism, I didn't want to reimplement that there so decided to violate the layering and put these (CM) class specific checks in the mad module.
-- Hal
Quoted 21 lines
Ira
Quoted 19 lines
drivers/infiniband/core/mad.c +0
-0/* Filter GSI packets sent to QP0 */ if (qp_num !=0 ) valid =1 ;include/rdma/ib_mad.h +2
-0@@ -237,6 +237,8 @@ struct ib_vendor_mad {u8 data[IB_MGMT_VENDOR_DATA ]; }; +#define IB_MGMT_CLASSPORTINFO_ATTR_ID cpu_to_be16(0x0001) +struct ib_class_port_info {u8 base_version;u8 class_version;Signature · 1 line
1.7.8.2
Quoted 5 lines
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html