From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hal Rosenstock Subject: Re: [PATCH] IB/mad: In validate_mad, validate CM method and attribute Date: Fri, 13 Nov 2015 14:09:09 -0500 Message-ID: <56463555.9000909@dev.mellanox.co.il> References: <56447CA2.1070802@dev.mellanox.co.il> <20151112181339.GA16488@phlsvsds.ph.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20151112181339.GA16488-W4f6Xiosr+yv7QzWx2u06xL4W9x8LtSr@public.gmane.org> Sender: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: "ira.weiny" Cc: Doug Ledford , "Hefty, Sean" , "linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Sagi Grimberg , Bart Van Assche List-Id: linux-rdma@vger.kernel.org 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 >> Signed-off-by: Hal Rosenstock >> --- >> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html