Skip to contentSkip to conversation

RDMA/irdma: Prevent user-triggered null deref on QP create

PATCH Applied Jacob Moroni 3 participants 3 messages Jun 17 – Jul 2

Subsystems (2)
InfiniBand Intel Ethernet protocol driver for RDMA
lore
Share & tools
lore

Jacob Moroni

author
#0 lore raw

PATCH 1/1 RDMA/irdma: Prevent user-triggered null deref on QP create

Previously, the user QP creation path would only attempt to populate iwqp->iwpbl if the user-provided req.user_wqe_bufs field was non-zero. The problem is that iwqp->iwpbl is unconditionally dereferenced later on in irdma_setup_virt_qp.

While there was a check for iwqp->iwpbl != NULL, this check would only occur if req.user_wqe_bufs was non-zero. The end result is that a user could send a zero user_wqe_bufs value and trigger a null ptr deref.

Fix this by unconditionally calling irdma_get_pbl and bailing if it fails, similar to the CQ and SRQ paths.

Fixes b48c24c2d710 ("RDMA/irdma: Implement device supported verb APIs")
Signed-off-by Jacob Moroni <[email protected]>
---
 drivers/infiniband/hw/irdma/verbs.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)
drivers/infiniband/hw/irdma/verbs.c +10 -11
index 4c0ea7c9b9..4124e4d732 100644
@@ -638,17 +638,16 @@ static int irdma_setup_umode_qp(struct ib_udata *udata,
 
 	iwqp->ctx_info.qp_compl_ctx = req.user_compl_ctx;
 	iwqp->user_mode = 1;
-	if (req.user_wqe_bufs) {
-		spin_lock_irqsave(&ucontext->qp_reg_mem_list_lock, flags);
-		iwqp->iwpbl = irdma_get_pbl((unsigned long)req.user_wqe_bufs,
-					    &ucontext->qp_reg_mem_list);
-		spin_unlock_irqrestore(&ucontext->qp_reg_mem_list_lock, flags);
-
-		if (!iwqp->iwpbl) {
-			ret = -ENODATA;
-			ibdev_dbg(&iwdev->ibdev, "VERBS: no pbl info\n");
-			return ret;
-		}
+
+	spin_lock_irqsave(&ucontext->qp_reg_mem_list_lock, flags);
+	iwqp->iwpbl = irdma_get_pbl((unsigned long)req.user_wqe_bufs,
+				    &ucontext->qp_reg_mem_list);
+	spin_unlock_irqrestore(&ucontext->qp_reg_mem_list_lock, flags);
+
+	if (!iwqp->iwpbl) {
+		ret = -ENODATA;
+		ibdev_dbg(&iwdev->ibdev, "VERBS: no pbl info\n");
+		return ret;
 	}
 
 	if (!ucontext->use_raw_attrs) {
Signature · 1 line
2.54.0.1189.g8c84645362-goog
Reviewed-by on v1 1/1
#1 lore raw
Quoted 19 lines · Jacob Moroni
drivers/infiniband/hw/irdma/verbs.c +0 -11
index 4c0ea7c9b9..4124e4d732 100644
@@ -638,17 +638,16 @@ static int irdma_setup_umode_qp(struct ib_udata *udata,
 
        iwqp->ctx_info.qp_compl_ctx = req.user_compl_ctx;
        iwqp->user_mode = 1;
-       if (req.user_wqe_bufs) {
-               spin_lock_irqsave(&ucontext->qp_reg_mem_list_lock, flags);
-               iwqp->iwpbl = irdma_get_pbl((unsigned long)req.user_wqe_bufs,
-                                           &ucontext->qp_reg_mem_list);
-               spin_unlock_irqrestore(&ucontext->qp_reg_mem_list_lock, flags);
-
-               if (!iwqp->iwpbl) {
-                       ret = -ENODATA;
-                       ibdev_dbg(&iwdev->ibdev, "VERBS: no pbl info\n");
-                       return ret;
-               }

drivers/infiniband/hw/irdma/verbs.c static int irdma_setup_umode_qp(struct ib_udata *udata, · line 650

Quoted 10 lines
drivers/infiniband/hw/irdma/verbs.c +10 -0
+
+       spin_lock_irqsave(&ucontext->qp_reg_mem_list_lock, flags);
+       iwqp->iwpbl = irdma_get_pbl((unsigned long)req.user_wqe_bufs,
+                                   &ucontext->qp_reg_mem_list);
+       spin_unlock_irqrestore(&ucontext->qp_reg_mem_list_lock, flags);
+
+       if (!iwqp->iwpbl) {
+               ret = -ENODATA;
+               ibdev_dbg(&iwdev->ibdev, "VERBS: no pbl info\n");
+               return ret;

Unconditional pbl lookup and bailing out early appears to be a clean fix without any leak. Taking the spinlock unconditionally in the slow path to validate input also appears appropriate. Guarding the check within irdma_setup_virt_qp() might be an alternative, but enforcing that a QP has a proper pbl upfront makes sense.

0x00000 does not appear to be a practical address in the real world. If it were, the original code would break anyway.

Nit: -EINVAL might be an alternative return for an invalid address.

Reviewed-by David Hu <[email protected]>
maintainer Applied
#2 lore raw
In reply to Jacob Moroni
Quoted 13 lines · Jacob Moroni

Previously, the user QP creation path would only attempt to populate iwqp->iwpbl if the user-provided req.user_wqe_bufs field was non-zero. The problem is that iwqp->iwpbl is unconditionally dereferenced later on in irdma_setup_virt_qp.

While there was a check for iwqp->iwpbl != NULL, this check would only occur if req.user_wqe_bufs was non-zero. The end result is that a user could send a zero user_wqe_bufs value and trigger a null ptr deref.

Fix this by unconditionally calling irdma_get_pbl and bailing if it fails, similar to the CQ and SRQ paths.

Quoted 6 lines
Fixes b48c24c2d710 ("RDMA/irdma: Implement device supported verb APIs")
Signed-off-by Jacob Moroni <[email protected]>
Reviewed-by David Hu <[email protected]>
---
 drivers/infiniband/hw/irdma/verbs.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

Applied to for-rc

Thanks,
Jason