[PATCH] IB/mad: In validate_mad, validate CM method and attribute

[PATCH] IB/mad: In validate_mad, validate CM method and attribute

[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.

Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Hal Rosenstock <hal-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 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;
+		}
 		/* 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

RE: [PATCH] IB/mad: In validate_mad, validate CM method and attribute

[Hefty, Sean]

> 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>

Re: [PATCH] IB/mad: In validate_mad, validate CM method and attribute

[Bart Van Assche]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/mad: In validate_mad, validate CM method and attribute

[ira.weiny]

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-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Hal Rosenstock <hal-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
>  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-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

RE: [PATCH] IB/mad: In validate_mad, validate CM method and attribute

[Hefty, Sean]

> > +		/* 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/mad: In validate_mad, validate CM method and attribute

[Jason Gunthorpe]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] IB/mad: In validate_mad, validate CM method and attribute

[Hefty, Sean]

> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/mad: In validate_mad, validate CM method and attribute

[Hal Rosenstock]

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-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>> Signed-off-by: Hal Rosenstock <hal-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>> ---
>>  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

Re: [PATCH] IB/mad: In validate_mad, validate CM method and attribute

[Hal Rosenstock]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/mad: In validate_mad, validate CM method and attribute

[Hal Rosenstock]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/mad: In validate_mad, validate CM method and attribute

[Hal Rosenstock]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/mad: In validate_mad, validate CM method and attribute

[Sagi Grimberg]

> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html