| Message ID | 56447CA2.1070802@dev.mellanox.co.il (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
> 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. > > Reviewed-by: Sagi Grimberg <sagig@mellanox.com> > Signed-off-by: Hal Rosenstock <hal@mellanox.com> Reviewed-by: Sean Hefty <sean.hefty@intel.com>
On 11/12/2015 03:48 AM, Hal Rosenstock wrote: > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 12, 2015 at 01:48:50PM +0200, Hal Rosenstock wrote: > > 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. > > Reviewed-by: Sagi Grimberg <sagig@mellanox.com> > Signed-off-by: Hal Rosenstock <hal@mellanox.com> > --- > drivers/infiniband/core/mad.c | 8 ++++++++ > include/rdma/ib_mad.h | 2 ++ > 2 files changed, 10 insertions(+), 0 deletions(-) > > diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c > index 8d8af7a..e2d425f 100644 > --- a/drivers/infiniband/core/mad.c > +++ b/drivers/infiniband/core/mad.c > @@ -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; > + } 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 > /* Filter GSI packets sent to QP0 */ > if (qp_num != 0) > valid = 1; > diff --git a/include/rdma/ib_mad.h b/include/rdma/ib_mad.h > index 188df91..ec9b44d 100644 > --- a/include/rdma/ib_mad.h > +++ b/include/rdma/ib_mad.h > @@ -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; > -- > 1.7.8.2 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > + /* 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? 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 12, 2015 at 08:30:55PM +0000, Hefty, Sean wrote: > > > + /* 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? > > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/12/2015 1:13 PM, ira.weiny wrote: > On Thu, Nov 12, 2015 at 01:48:50PM +0200, Hal Rosenstock wrote: >> >> 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. >> >> Reviewed-by: Sagi Grimberg <sagig@mellanox.com> >> Signed-off-by: Hal Rosenstock <hal@mellanox.com> >> --- >> drivers/infiniband/core/mad.c | 8 ++++++++ >> include/rdma/ib_mad.h | 2 ++ >> 2 files changed, 10 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c >> index 8d8af7a..e2d425f 100644 >> --- a/drivers/infiniband/core/mad.c >> +++ b/drivers/infiniband/core/mad.c >> @@ -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; >> + } > > 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... > 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 > Ira > >> /* Filter GSI packets sent to QP0 */ >> if (qp_num != 0) >> valid = 1; >> diff --git a/include/rdma/ib_mad.h b/include/rdma/ib_mad.h >> index 188df91..ec9b44d 100644 >> --- a/include/rdma/ib_mad.h >> +++ b/include/rdma/ib_mad.h >> @@ -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; >> -- >> 1.7.8.2 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in >> the body of a message to majordomo@vger.kernel.org >> 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
IOn 11/12/2015 5:13 PM, Jason Gunthorpe wrote: > On Thu, Nov 12, 2015 at 08:30:55PM +0000, Hefty, Sean wrote: >>>> + /* 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? >> >> I believe this does. My bad. That wasn't my intention. >> 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. > 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 > Jason > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/12/2015 5:57 PM, Hefty, Sean wrote: >> 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/12/2015 3:30 PM, Hefty, Sean wrote: >>> + /* 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? > > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c index 8d8af7a..e2d425f 100644 --- a/drivers/infiniband/core/mad.c +++ b/drivers/infiniband/core/mad.c @@ -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; diff --git a/include/rdma/ib_mad.h b/include/rdma/ib_mad.h index 188df91..ec9b44d 100644 --- a/include/rdma/ib_mad.h +++ b/include/rdma/ib_mad.h @@ -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;