[PATCH rdma-next] IB/core: Only maintain real QPs in the security lists

[PATCH rdma-next] IB/core: Only maintain real QPs in the security lists

[Leon Romanovsky]

From: Daniel Jurgens <danielj@mellanox.com>

When modify QP is called on a shared QP update the security context for
the real QP. When security is subsequently enforced the shared QP
handles will be checked as well.

Without this change shared QP handles get added to the port/pkey lists,
which is a bug, because not all shared QP handles will be checked for
access. Also the shared QP security context wouldn't get removed from
the port/pkey lists causing access to free memory and list corruption
when they are destroyed.

Cc: stable@vger.kernel.org
Fixes: d291f1a65232 ("IB/core: Enforce PKey security on QPs")
Signed-off-by: Daniel Jurgens <danielj@mellanox.com>
Reviewed-by: Parav Pandit <parav@mellanox.com>
Signed-off-by: Leon Romanovsky <leon@kernel.org>
---
 drivers/infiniband/core/security.c | 51 +++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 23 deletions(-)

diff --git a/drivers/infiniband/core/security.c b/drivers/infiniband/core/security.c
index 455cba7f9640..23278ed5be45 100644
--- a/drivers/infiniband/core/security.c
+++ b/drivers/infiniband/core/security.c
@@ -87,16 +87,14 @@ static int enforce_qp_pkey_security(u16 pkey,
 	if (ret)
 		return ret;

-	if (qp_sec->qp == qp_sec->qp->real_qp) {
-		list_for_each_entry(shared_qp_sec,
-				    &qp_sec->shared_qp_list,
-				    shared_qp_list) {
-			ret = security_ib_pkey_access(shared_qp_sec->security,
-						      subnet_prefix,
-						      pkey);
-			if (ret)
-				return ret;
-		}
+	list_for_each_entry(shared_qp_sec,
+			    &qp_sec->shared_qp_list,
+			    shared_qp_list) {
+		ret = security_ib_pkey_access(shared_qp_sec->security,
+					      subnet_prefix,
+					      pkey);
+		if (ret)
+			return ret;
 	}
 	return 0;
 }
@@ -560,15 +558,22 @@ int ib_security_modify_qp(struct ib_qp *qp,
 	int ret = 0;
 	struct ib_ports_pkeys *tmp_pps;
 	struct ib_ports_pkeys *new_pps;
-	bool special_qp = (qp->qp_type == IB_QPT_SMI ||
-			   qp->qp_type == IB_QPT_GSI ||
-			   qp->qp_type >= IB_QPT_RESERVED1);
+	struct ib_qp *real_qp = qp->real_qp;
+	bool special_qp = (real_qp->qp_type == IB_QPT_SMI ||
+			   real_qp->qp_type == IB_QPT_GSI ||
+			   real_qp->qp_type >= IB_QPT_RESERVED1);
 	bool pps_change = ((qp_attr_mask & (IB_QP_PKEY_INDEX | IB_QP_PORT)) ||
 			   (qp_attr_mask & IB_QP_ALT_PATH));

+	/* The port/pkey settings are maintained only for the real QP. Open
+	 * handles on the real QP will be in the shared_qp_list. When
+	 * enforcing security on the real QP all the shared QPs will be
+	 * checked as well.
+	 */
+
 	if (pps_change && !special_qp) {
-		mutex_lock(&qp->qp_sec->mutex);
-		new_pps = get_new_pps(qp,
+		mutex_lock(&real_qp->qp_sec->mutex);
+		new_pps = get_new_pps(real_qp,
 				      qp_attr,
 				      qp_attr_mask);

@@ -586,14 +591,14 @@ int ib_security_modify_qp(struct ib_qp *qp,

 		if (!ret)
 			ret = check_qp_port_pkey_settings(new_pps,
-							  qp->qp_sec);
+							  real_qp->qp_sec);
 	}

 	if (!ret)
-		ret = qp->device->modify_qp(qp->real_qp,
-					    qp_attr,
-					    qp_attr_mask,
-					    udata);
+		ret = real_qp->device->modify_qp(real_qp,
+						 qp_attr,
+						 qp_attr_mask,
+						 udata);

 	if (pps_change && !special_qp) {
 		/* Clean up the lists and free the appropriate
@@ -602,8 +607,8 @@ int ib_security_modify_qp(struct ib_qp *qp,
 		if (ret) {
 			tmp_pps = new_pps;
 		} else {
-			tmp_pps = qp->qp_sec->ports_pkeys;
-			qp->qp_sec->ports_pkeys = new_pps;
+			tmp_pps = real_qp->qp_sec->ports_pkeys;
+			real_qp->qp_sec->ports_pkeys = new_pps;
 		}

 		if (tmp_pps) {
@@ -611,7 +616,7 @@ int ib_security_modify_qp(struct ib_qp *qp,
 			port_pkey_list_remove(&tmp_pps->alt);
 		}
 		kfree(tmp_pps);
-		mutex_unlock(&qp->qp_sec->mutex);
+		mutex_unlock(&real_qp->qp_sec->mutex);
 	}
 	return ret;
 }

Re: [PATCH rdma-next] IB/core: Only maintain real QPs in the security lists

[Jason Gunthorpe]

On Tue, Nov 07, 2017 at 06:33:26PM +0200, Leon Romanovsky wrote:

> -	bool special_qp = (qp->qp_type == IB_QPT_SMI ||
> -			   qp->qp_type == IB_QPT_GSI ||
> -			   qp->qp_type >= IB_QPT_RESERVED1);
> +	struct ib_qp *real_qp = qp->real_qp;
> +	bool special_qp = (real_qp->qp_type == IB_QPT_SMI ||
> +			   real_qp->qp_type == IB_QPT_GSI ||
> +			   real_qp->qp_type >= IB_QPT_RESERVED1);

This QPT_RESERVED stuff was not supposed to be visible to the core
layer, so why are we adding checks in security????

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 rdma-next] IB/core: Only maintain real QPs in the security lists

[Daniel Jurgens]

On 11/7/2017 10:46 AM, Jason Gunthorpe wrote:
> On Tue, Nov 07, 2017 at 06:33:26PM +0200, Leon Romanovsky wrote:
>
>> -	bool special_qp = (qp->qp_type == IB_QPT_SMI ||
>> -			   qp->qp_type == IB_QPT_GSI ||
>> -			   qp->qp_type >= IB_QPT_RESERVED1);
>> +	struct ib_qp *real_qp = qp->real_qp;
>> +	bool special_qp = (real_qp->qp_type == IB_QPT_SMI ||
>> +			   real_qp->qp_type == IB_QPT_GSI ||
>> +			   real_qp->qp_type >= IB_QPT_RESERVED1);
> This QPT_RESERVED stuff was not supposed to be visible to the core
> layer, so why are we adding checks in security????
>
> Jason

The checks exclude those QPs from security enforcement.  They've been there the whole time, you reviewed this previously right?

Re: [PATCH rdma-next] IB/core: Only maintain real QPs in the security lists

[Jason Gunthorpe]

On Tue, Nov 07, 2017 at 10:52:11AM -0600, Daniel Jurgens wrote:
> On 11/7/2017 10:46 AM, Jason Gunthorpe wrote:
> > On Tue, Nov 07, 2017 at 06:33:26PM +0200, Leon Romanovsky wrote:
> >
> >> -	bool special_qp = (qp->qp_type == IB_QPT_SMI ||
> >> -			   qp->qp_type == IB_QPT_GSI ||
> >> -			   qp->qp_type >= IB_QPT_RESERVED1);
> >> +	struct ib_qp *real_qp = qp->real_qp;
> >> +	bool special_qp = (real_qp->qp_type == IB_QPT_SMI ||
> >> +			   real_qp->qp_type == IB_QPT_GSI ||
> >> +			   real_qp->qp_type >= IB_QPT_RESERVED1);
> > This QPT_RESERVED stuff was not supposed to be visible to the core
> > layer, so why are we adding checks in security????
> 
> The checks exclude those QPs from security enforcement.  They've
> been there the whole time, you reviewed this previously right?

Nope

.. and they shouldn't be there, those reserved QPTs are totally banned
from user space so it shouldn't matter for security

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 rdma-next] IB/core: Only maintain real QPs in the security lists

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1215 bytes --]

On Tue, Nov 07, 2017 at 09:55:39AM -0700, Jason Gunthorpe wrote:
> On Tue, Nov 07, 2017 at 10:52:11AM -0600, Daniel Jurgens wrote:
> > On 11/7/2017 10:46 AM, Jason Gunthorpe wrote:
> > > On Tue, Nov 07, 2017 at 06:33:26PM +0200, Leon Romanovsky wrote:
> > >
> > >> -	bool special_qp = (qp->qp_type == IB_QPT_SMI ||
> > >> -			   qp->qp_type == IB_QPT_GSI ||
> > >> -			   qp->qp_type >= IB_QPT_RESERVED1);
> > >> +	struct ib_qp *real_qp = qp->real_qp;
> > >> +	bool special_qp = (real_qp->qp_type == IB_QPT_SMI ||
> > >> +			   real_qp->qp_type == IB_QPT_GSI ||
> > >> +			   real_qp->qp_type >= IB_QPT_RESERVED1);
> > > This QPT_RESERVED stuff was not supposed to be visible to the core
> > > layer, so why are we adding checks in security????
> >
> > The checks exclude those QPs from security enforcement.  They've
> > been there the whole time, you reviewed this previously right?
>
> Nope
>
> .. and they shouldn't be there, those reserved QPTs are totally banned
> from user space so it shouldn't matter for security

We will send followup patch to clean it, but for this patch I prefer do
not change the logic here because it is supposed to go to stable@.

Thanks

>
> Jason
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-next] IB/core: Only maintain real QPs in the security lists

[Jason Gunthorpe]

On Tue, Nov 07, 2017 at 07:10:26PM +0200, Leon Romanovsky wrote:

> We will send followup patch to clean it, but for this patch I prefer do
> not change the logic here because it is supposed to go to stable@.

Yep

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 rdma-next] IB/core: Only maintain real QPs in the security lists

[Daniel Jurgens]

On 11/7/2017 10:55 AM, Jason Gunthorpe wrote:
> On Tue, Nov 07, 2017 at 10:52:11AM -0600, Daniel Jurgens wrote:
>> On 11/7/2017 10:46 AM, Jason Gunthorpe wrote:
>>> On Tue, Nov 07, 2017 at 06:33:26PM +0200, Leon Romanovsky wrote:
>>>
>>>> -	bool special_qp = (qp->qp_type == IB_QPT_SMI ||
>>>> -			   qp->qp_type == IB_QPT_GSI ||
>>>> -			   qp->qp_type >= IB_QPT_RESERVED1);
>>>> +	struct ib_qp *real_qp = qp->real_qp;
>>>> +	bool special_qp = (real_qp->qp_type == IB_QPT_SMI ||
>>>> +			   real_qp->qp_type == IB_QPT_GSI ||
>>>> +			   real_qp->qp_type >= IB_QPT_RESERVED1);
>>> This QPT_RESERVED stuff was not supposed to be visible to the core
>>> layer, so why are we adding checks in security????
>> The checks exclude those QPs from security enforcement.  They've
>> been there the whole time, you reviewed this previously right?
> Nope
>
> .. and they shouldn't be there, those reserved QPTs are totally banned
> from user space so it shouldn't matter for security
>
> Jason
>
This flow is through ib_modify_qp, it's not user space specific.  If it really pains you that much it can be changed an inclusive compare instead of exclusive. This discussion isn't relevant  to the content of the patch in question though.

Re: [PATCH rdma-next] IB/core: Only maintain real QPs in the security lists

[Jason Gunthorpe]

On Tue, Nov 07, 2017 at 11:15:12AM -0600, Daniel Jurgens wrote:

> This flow is through ib_modify_qp, it's not user space specific. If
> it really pains you that much it can be changed an inclusive compare
> instead of exclusive.

There was already a RFC on re-purpsing those constants and exposing
them to user space.

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 rdma-next] IB/core: Only maintain real QPs in the security lists

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 734 bytes --]

On Tue, Nov 07, 2017 at 10:32:15AM -0700, Jason Gunthorpe wrote:
> On Tue, Nov 07, 2017 at 11:15:12AM -0600, Daniel Jurgens wrote:
>
> > This flow is through ib_modify_qp, it's not user space specific. If
> > it really pains you that much it can be changed an inclusive compare
> > instead of exclusive.
>
> There was already a RFC on re-purpsing those constants and exposing
> them to user space.

It pains me, the flows of create_qp/modify_qp are completely
unmaintainable mess and I would like to see more explicit checks, so
refactoring will be easy. We will reach that step much sooner than I expected.

Thanks

BTW, I spent last day to annotate all different paths of create_qp for
the resource tracking information.

>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-next] IB/core: Only maintain real QPs in the security lists

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1043 bytes --]

On Tue, 2017-11-07 at 18:33 +0200, Leon Romanovsky wrote:
> From: Daniel Jurgens <danielj@mellanox.com>
> 
> When modify QP is called on a shared QP update the security context for
> the real QP. When security is subsequently enforced the shared QP
> handles will be checked as well.
> 
> Without this change shared QP handles get added to the port/pkey lists,
> which is a bug, because not all shared QP handles will be checked for
> access. Also the shared QP security context wouldn't get removed from
> the port/pkey lists causing access to free memory and list corruption
> when they are destroyed.
> 
> Cc: stable@vger.kernel.org
> Fixes: d291f1a65232 ("IB/core: Enforce PKey security on QPs")
> Signed-off-by: Daniel Jurgens <danielj@mellanox.com>
> Reviewed-by: Parav Pandit <parav@mellanox.com>
> Signed-off-by: Leon Romanovsky <leon@kernel.org>

Thanks, applied.


-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]