diff mbox series

[opensm] osm_sa_mcmember_record.c: Fix use after free in mcmr_rcv_join_mgrp

Message ID c20f68c4-bc1d-1304-3d41-1a8cd7c3fd89@dev.mellanox.co.il (mailing list archive)
State Not Applicable
Headers show
Series [opensm] osm_sa_mcmember_record.c: Fix use after free in mcmr_rcv_join_mgrp | expand

Commit Message

Hal Rosenstock Aug. 7, 2018, 1:44 p.m. UTC 
The cleanup function frees p_mgrp before the osm_log() prints the GUID.

Issue was found by Coverity.

Error: USE_AFTER_FREE (CWE-825): [#def11]
opensm-3.3.20/opensm/osm_sa_mcmember_record.c:1340: freed_arg: "osm_mgrp_cleanup" frees "p_mgrp".
opensm-3.3.20/opensm/osm_multicast.c:184:2: freed_arg: "free" frees parameter "mgrp".
opensm-3.3.20/opensm/osm_sa_mcmember_record.c:1342: deref_after_free: Dereferencing freed pointer "p_mgrp".
|# 1340|   			osm_mgrp_cleanup(sa->p_subn, p_mgrp);
|# 1341|   		CL_PLOCK_RELEASE(sa->p_lock);
|# 1342|-> 		OSM_LOG(sa->p_log, OSM_LOG_ERROR, "ERR 1B12: "
|# 1343|   			"validate_more_comp_fields, validate_port_caps, "
|# 1344|   			"or JoinState = 0 failed for MGID: %s port 0x%016" PRIx64

Error: USE_AFTER_FREE (CWE-825): [#def12]
opensm-3.3.20/opensm/osm_sa_mcmember_record.c:1357: freed_arg: "osm_mgrp_cleanup" frees "p_mgrp".
opensm-3.3.20/opensm/osm_multicast.c:184:2: freed_arg: "free" frees parameter "mgrp".
opensm-3.3.20/opensm/osm_sa_mcmember_record.c:1360: deref_after_free: Dereferencing freed pointer "p_mgrp".
|# 1358|   		CL_PLOCK_RELEASE(sa->p_lock);
|# 1359|   		memset(gid_str, 0, sizeof(gid_str));
|# 1360|-> 		OSM_LOG(sa->p_log, OSM_LOG_ERROR, "ERR 1B14: "
|# 1361|   			"Cannot join port 0x%016" PRIx64 " to MGID %s - "
|# 1362|

Signed-off-by: Dan Ben Yosef <danby@mellanox.com>
Signed-off-by: Alex Netes <alexne@mellanox.com>
Signed-off-by: Hal Rosenstock <hal@mellanox.com>
---
 opensm/osm_sa_mcmember_record.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Honggang LI Aug. 7, 2018, 11:33 p.m. UTC | #1 
On Tue, Aug 07, 2018 at 09:44:03AM -0400, Hal Rosenstock wrote:
> 
> The cleanup function frees p_mgrp before the osm_log() prints the GUID.
> 
> Issue was found by Coverity.
> 
> Error: USE_AFTER_FREE (CWE-825): [#def11]
> opensm-3.3.20/opensm/osm_sa_mcmember_record.c:1340: freed_arg: "osm_mgrp_cleanup" frees "p_mgrp".
> opensm-3.3.20/opensm/osm_multicast.c:184:2: freed_arg: "free" frees parameter "mgrp".
> opensm-3.3.20/opensm/osm_sa_mcmember_record.c:1342: deref_after_free: Dereferencing freed pointer "p_mgrp".
> |# 1340|   			osm_mgrp_cleanup(sa->p_subn, p_mgrp);
> |# 1341|   		CL_PLOCK_RELEASE(sa->p_lock);
> |# 1342|-> 		OSM_LOG(sa->p_log, OSM_LOG_ERROR, "ERR 1B12: "
> |# 1343|   			"validate_more_comp_fields, validate_port_caps, "
> |# 1344|   			"or JoinState = 0 failed for MGID: %s port 0x%016" PRIx64
> 

This patch fixes #def11, but does not fix #def12

NAK and REJECT.

> Error: USE_AFTER_FREE (CWE-825): [#def12]
> opensm-3.3.20/opensm/osm_sa_mcmember_record.c:1357: freed_arg: "osm_mgrp_cleanup" frees "p_mgrp".
> opensm-3.3.20/opensm/osm_multicast.c:184:2: freed_arg: "free" frees parameter "mgrp".
> opensm-3.3.20/opensm/osm_sa_mcmember_record.c:1360: deref_after_free: Dereferencing freed pointer "p_mgrp".
> |# 1358|   		CL_PLOCK_RELEASE(sa->p_lock);
> |# 1359|   		memset(gid_str, 0, sizeof(gid_str));
> |# 1360|-> 		OSM_LOG(sa->p_log, OSM_LOG_ERROR, "ERR 1B14: "
> |# 1361|   			"Cannot join port 0x%016" PRIx64 " to MGID %s - "
> |# 1362|

This issue persists.

> 
> Signed-off-by: Dan Ben Yosef <danby@mellanox.com>
> Signed-off-by: Alex Netes <alexne@mellanox.com>
> Signed-off-by: Hal Rosenstock <hal@mellanox.com>
> ---
>  opensm/osm_sa_mcmember_record.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/opensm/osm_sa_mcmember_record.c b/opensm/osm_sa_mcmember_record.c
> index 9b94993..e113416 100644
> --- a/opensm/osm_sa_mcmember_record.c
> +++ b/opensm/osm_sa_mcmember_record.c
> @@ -1345,6 +1345,12 @@ static void mcmr_rcv_join_mgrp(IN osm_sa_t * sa, IN osm_madw_t * p_madw)
>  	    || !validate_port_caps(sa->p_log, p_mgrp, p_physp)
>  	    || !(join_state != 0)) {
>  		char gid_str[INET6_ADDRSTRLEN];
> +		memset(gid_str, 0, sizeof(gid_str));
> +
> +		/* get the gid_str before the cleanup, the cleanup can free the pointer */
> +		inet_ntop(AF_INET6, p_mgrp->mcmember_rec.mgid.raw, gid_str,
> +			  sizeof gid_str);
> +
>  		/* since we might have created the new group we need to cleanup */
>  		if (is_new_group)
>  			osm_mgrp_cleanup(sa->p_subn, p_mgrp);
> @@ -1353,9 +1359,7 @@ static void mcmr_rcv_join_mgrp(IN osm_sa_t * sa, IN osm_madw_t * p_madw)
>  			"validate_more_comp_fields, validate_port_caps, "
>  			"or JoinState = 0 failed for MGID: %s port 0x%016" PRIx64
>  			" (%s), sending IB_SA_MAD_STATUS_REQ_INVALID\n",
> -			   inet_ntop(AF_INET6, p_mgrp->mcmember_rec.mgid.raw,
> -				     gid_str, sizeof gid_str),
> -			cl_ntoh64(portguid), p_port->p_node->print_desc);
> +			gid_str, cl_ntoh64(portguid), p_port->p_node->print_desc);
>  		osm_sa_send_error(sa, p_madw, IB_SA_MAD_STATUS_REQ_INVALID);
>  		goto Exit;
>  	}
> -- 
> 2.8.4
> 
--
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
Hal Rosenstock Aug. 8, 2018, 9:27 a.m. UTC | #2 
On 8/7/2018 7:33 PM, Honggang LI wrote:
> On Tue, Aug 07, 2018 at 09:44:03AM -0400, Hal Rosenstock wrote:
>>
>> The cleanup function frees p_mgrp before the osm_log() prints the GUID.
>>
>> Issue was found by Coverity.
>>
>> Error: USE_AFTER_FREE (CWE-825): [#def11]
>> opensm-3.3.20/opensm/osm_sa_mcmember_record.c:1340: freed_arg: "osm_mgrp_cleanup" frees "p_mgrp".
>> opensm-3.3.20/opensm/osm_multicast.c:184:2: freed_arg: "free" frees parameter "mgrp".
>> opensm-3.3.20/opensm/osm_sa_mcmember_record.c:1342: deref_after_free: Dereferencing freed pointer "p_mgrp".
>> |# 1340|   			osm_mgrp_cleanup(sa->p_subn, p_mgrp);
>> |# 1341|   		CL_PLOCK_RELEASE(sa->p_lock);
>> |# 1342|-> 		OSM_LOG(sa->p_log, OSM_LOG_ERROR, "ERR 1B12: "
>> |# 1343|   			"validate_more_comp_fields, validate_port_caps, "
>> |# 1344|   			"or JoinState = 0 failed for MGID: %s port 0x%016" PRIx64
>>
> 
> This patch fixes #def11, but does not fix #def12
> 
> NAK and REJECT.

Sorry. I missed update of second case. Updated patch to follow shortly.

>> Error: USE_AFTER_FREE (CWE-825): [#def12]
>> opensm-3.3.20/opensm/osm_sa_mcmember_record.c:1357: freed_arg: "osm_mgrp_cleanup" frees "p_mgrp".
>> opensm-3.3.20/opensm/osm_multicast.c:184:2: freed_arg: "free" frees parameter "mgrp".
>> opensm-3.3.20/opensm/osm_sa_mcmember_record.c:1360: deref_after_free: Dereferencing freed pointer "p_mgrp".
>> |# 1358|   		CL_PLOCK_RELEASE(sa->p_lock);
>> |# 1359|   		memset(gid_str, 0, sizeof(gid_str));
>> |# 1360|-> 		OSM_LOG(sa->p_log, OSM_LOG_ERROR, "ERR 1B14: "
>> |# 1361|   			"Cannot join port 0x%016" PRIx64 " to MGID %s - "
>> |# 1362|
> 
> This issue persists.
> 
>>
>> Signed-off-by: Dan Ben Yosef <danby@mellanox.com>
>> Signed-off-by: Alex Netes <alexne@mellanox.com>
>> Signed-off-by: Hal Rosenstock <hal@mellanox.com>
>> ---
>>  opensm/osm_sa_mcmember_record.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/opensm/osm_sa_mcmember_record.c b/opensm/osm_sa_mcmember_record.c
>> index 9b94993..e113416 100644
>> --- a/opensm/osm_sa_mcmember_record.c
>> +++ b/opensm/osm_sa_mcmember_record.c
>> @@ -1345,6 +1345,12 @@ static void mcmr_rcv_join_mgrp(IN osm_sa_t * sa, IN osm_madw_t * p_madw)
>>  	    || !validate_port_caps(sa->p_log, p_mgrp, p_physp)
>>  	    || !(join_state != 0)) {
>>  		char gid_str[INET6_ADDRSTRLEN];
>> +		memset(gid_str, 0, sizeof(gid_str));
>> +
>> +		/* get the gid_str before the cleanup, the cleanup can free the pointer */
>> +		inet_ntop(AF_INET6, p_mgrp->mcmember_rec.mgid.raw, gid_str,
>> +			  sizeof gid_str);
>> +
>>  		/* since we might have created the new group we need to cleanup */
>>  		if (is_new_group)
>>  			osm_mgrp_cleanup(sa->p_subn, p_mgrp);
>> @@ -1353,9 +1359,7 @@ static void mcmr_rcv_join_mgrp(IN osm_sa_t * sa, IN osm_madw_t * p_madw)
>>  			"validate_more_comp_fields, validate_port_caps, "
>>  			"or JoinState = 0 failed for MGID: %s port 0x%016" PRIx64
>>  			" (%s), sending IB_SA_MAD_STATUS_REQ_INVALID\n",
>> -			   inet_ntop(AF_INET6, p_mgrp->mcmember_rec.mgid.raw,
>> -				     gid_str, sizeof gid_str),
>> -			cl_ntoh64(portguid), p_port->p_node->print_desc);
>> +			gid_str, cl_ntoh64(portguid), p_port->p_node->print_desc);
>>  		osm_sa_send_error(sa, p_madw, IB_SA_MAD_STATUS_REQ_INVALID);
>>  		goto Exit;
>>  	}
>> -- 
>> 2.8.4
>>
> 
--
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 mbox series

Patch

diff --git a/opensm/osm_sa_mcmember_record.c b/opensm/osm_sa_mcmember_record.c
index 9b94993..e113416 100644
--- a/opensm/osm_sa_mcmember_record.c
+++ b/opensm/osm_sa_mcmember_record.c
@@ -1345,6 +1345,12 @@  static void mcmr_rcv_join_mgrp(IN osm_sa_t * sa, IN osm_madw_t * p_madw)
 	    || !validate_port_caps(sa->p_log, p_mgrp, p_physp)
 	    || !(join_state != 0)) {
 		char gid_str[INET6_ADDRSTRLEN];
+		memset(gid_str, 0, sizeof(gid_str));
+
+		/* get the gid_str before the cleanup, the cleanup can free the pointer */
+		inet_ntop(AF_INET6, p_mgrp->mcmember_rec.mgid.raw, gid_str,
+			  sizeof gid_str);
+
 		/* since we might have created the new group we need to cleanup */
 		if (is_new_group)
 			osm_mgrp_cleanup(sa->p_subn, p_mgrp);
@@ -1353,9 +1359,7 @@  static void mcmr_rcv_join_mgrp(IN osm_sa_t * sa, IN osm_madw_t * p_madw)
 			"validate_more_comp_fields, validate_port_caps, "
 			"or JoinState = 0 failed for MGID: %s port 0x%016" PRIx64
 			" (%s), sending IB_SA_MAD_STATUS_REQ_INVALID\n",
-			   inet_ntop(AF_INET6, p_mgrp->mcmember_rec.mgid.raw,
-				     gid_str, sizeof gid_str),
-			cl_ntoh64(portguid), p_port->p_node->print_desc);
+			gid_str, cl_ntoh64(portguid), p_port->p_node->print_desc);
 		osm_sa_send_error(sa, p_madw, IB_SA_MAD_STATUS_REQ_INVALID);
 		goto Exit;
 	}