Skip to contentSkip to conversation

RDMA/ucma: Protect kernel from QPN larger than declared in IBTA

PATCH In review Leon Romanovsky 4 participants 6 messages Oct 28, 2019 – Oct 31, 2019 InfiniBand

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 RDMA/ucma: Protect kernel from QPN larger than declared in IBTA

From: Leon Romanovsky <[email protected]>

IBTA declares QPN as 24bits, mask input to ensure that kernel doesn't get higher bits.

Fixes 75216638572f ("RDMA/cma: Export rdma cm interface to userspace")

---
* Not fully tested yet, passed sanity tests for now.

---
 drivers/infiniband/core/ucma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/core/ucma.c +1 -1
index 0274e9b704be..57e68491a2fd 100644
@@ -1045,7 +1045,7 @@ static void ucma_copy_conn_param(struct rdma_cm_id *id,
 	dst->retry_count = src->retry_count;
 	dst->rnr_retry_count = src->rnr_retry_count;
 	dst->srq = src->srq;
-	dst->qp_num = src->qp_num;
+	dst->qp_num = src->qp_num & 0xFFFFFF;
 	dst->qkey = (id->route.addr.src_addr.ss_family == AF_IB) ? src->qkey : 0;
 }
 

--
2.20.1

#1 lore raw
Quoted 15 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

IBTA declares QPN as 24bits, mask input to ensure that kernel doesn't get higher bits.

Fixes 75216638572f ("RDMA/cma: Export rdma cm interface to userspace")

* Not fully tested yet, passed sanity tests for now.

 drivers/infiniband/core/ucma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/core/ucma.c +1 -0
index 0274e9b704be..57e68491a2fd 100644
+++ b/drivers/infiniband/core/ucma.c

drivers/infiniband/core/ucma.c static void ucma_copy_conn_param(struct rdma_cm_id *id, · line 1047

Quoted 3 lines
drivers/infiniband/core/ucma.c +0 -0
@@ -1045,7 +1045,7 @@ static void ucma_copy_conn_param(struct rdma_cm_id *id,
 	dst->retry_count = src->retry_count;
 	dst->rnr_retry_count = src->rnr_retry_count;
 	dst->srq = src->srq;

srq too?

Jason

#2 lore raw
Quoted 12 lines · Jason Gunthorpe
Quoted 11 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

IBTA declares QPN as 24bits, mask input to ensure that kernel doesn't get higher bits.

Fixes 75216638572f ("RDMA/cma: Export rdma cm interface to userspace")

* Not fully tested yet, passed sanity tests for now.

 drivers/infiniband/core/ucma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 9 lines

drivers/infiniband/core/ucma.c static void ucma_copy_conn_param(struct rdma_cm_id *id, · line 1047

Quoted 7 lines
drivers/infiniband/core/ucma.c +1 -0
index 0274e9b704be..57e68491a2fd 100644
+++ b/drivers/infiniband/core/ucma.c
@@ -1045,7 +1045,7 @@ static void ucma_copy_conn_param(struct rdma_cm_id *id,
 	dst->retry_count = src->retry_count;
 	dst->rnr_retry_count = src->rnr_retry_count;
 	dst->srq = src->srq;

srq too?

If I read IBTA correctly, the answer is yes. In all CM messages, QPN, SRQ number and EECN are 24 bits.

Thanks

Quoted 2 lines

Jason

#3 lore raw
In reply to Leon Romanovsky
Quoted 16 lines

IBTA declares QPN as 24bits, mask input to ensure that kernel doesn't get higher bits.

Fixes 75216638572f ("RDMA/cma: Export rdma cm interface to userspace")

---
* Not fully tested yet, passed sanity tests for now.

---
 drivers/infiniband/core/ucma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/core/ucma.c +0 -0
index 0274e9b704be..57e68491a2fd 100644

drivers/infiniband/core/ucma.c static void ucma_copy_conn_param(struct rdma_cm_id *id, · line 1048

Quoted 5 lines
drivers/infiniband/core/ucma.c +1 -1
@@ -1045,7 +1045,7 @@ static void ucma_copy_conn_param(struct rdma_cm_id *id,
 	dst->retry_count = src->retry_count;
 	dst->rnr_retry_count = src->rnr_retry_count;
 	dst->srq = src->srq;
-	dst->qp_num = src->qp_num;
+	dst->qp_num = src->qp_num & 0xFFFFFF;

Why not isolate IBTA restrictions in the ib_cm?

#4 lore raw
Quoted 11 lines · Sean
Quoted 11 lines

IBTA declares QPN as 24bits, mask input to ensure that kernel doesn't get higher bits.

Fixes 75216638572f ("RDMA/cma: Export rdma cm interface to userspace")

---
* Not fully tested yet, passed sanity tests for now.

---
 drivers/infiniband/core/ucma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 12 lines

drivers/infiniband/core/ucma.c static void ucma_copy_conn_param(struct rdma_cm_id *id, · line 1048

Quoted 10 lines
drivers/infiniband/core/ucma.c +1 -1
index 0274e9b704be..57e68491a2fd 100644
@@ -1045,7 +1045,7 @@ static void ucma_copy_conn_param(struct rdma_cm_id *id,
 	dst->retry_count = src->retry_count;
 	dst->rnr_retry_count = src->rnr_retry_count;
 	dst->srq = src->srq;
-	dst->qp_num = src->qp_num;
+	dst->qp_num = src->qp_num & 0xFFFFFF;

Why not isolate IBTA restrictions in the ib_cm?

AFAIK, there are many places in IB/core code which assume that type of restriction, so it is safer and cleaner to sanitize input as early as possible.

Thanks

#5 lore raw
In reply to Leon Romanovsky
Quoted 18 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

IBTA declares QPN as 24bits, mask input to ensure that kernel doesn't get higher bits.

Fixes 75216638572f ("RDMA/cma: Export rdma cm interface to userspace")

---
* Not fully tested yet, passed sanity tests for now.

---
 drivers/infiniband/core/ucma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/core/ucma.c +0 -0
index 0274e9b704be..57e68491a2fd 100644

drivers/infiniband/core/ucma.c static void ucma_copy_conn_param(struct rdma_cm_id *id, · line 1050

Quoted 7 lines
drivers/infiniband/core/ucma.c +1 -1
@@ -1045,7 +1045,7 @@ static void ucma_copy_conn_param(struct rdma_cm_id *id,
 	dst->retry_count = src->retry_count;
 	dst->rnr_retry_count = src->rnr_retry_count;
 	dst->srq = src->srq;
-	dst->qp_num = src->qp_num;
+	dst->qp_num = src->qp_num & 0xFFFFFF;
 	dst->qkey = (id->route.addr.src_addr.ss_family == AF_IB) ? src->qkey : 0;
 }

This really needs to be squashed into the other qpn patch because what is really being proposed here is to move the masking from the core code to the ucma and the core code will assume that the caller is using correct QPNS. Maybe leave behind a WARN_ON to confirm this.

Jason