Skip to contentSkip to conversation

nvmet-rdma: Fix response use after free

PATCH Superseded Israel Rukshin 4 participants 7 messages Dec 3, 2018 – Dec 5, 2018

Subsystems (2)
NVMe NVMe target

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Israel Rukshin

author
#0 lore raw

PATCH 1/2 nvmet-rdma: Fix response use after free

nvmet_rdma_release_rsp() may free the response before using it at error flow.

Fixes 8407879 ("nvmet-rdma: fix possible bogus dereference under heavy load")
Signed-off-by Israel Rukshin <israelr at mellanox.com>
Reviewed-by Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/target/rdma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/nvme/target/rdma.c +2 -1
index 3f7971d3..732806a 100644
@@ -529,6 +529,7 @@ static void nvmet_rdma_send_done(struct ib_cq *cq, struct ib_wc *wc)
 {
 	struct nvmet_rdma_rsp *rsp =
 		container_of(wc->wr_cqe, struct nvmet_rdma_rsp, send_cqe);
+	struct nvmet_rdma_queue *queue = rsp->queue;
 
 	nvmet_rdma_release_rsp(rsp);
 

@@ -536,7 +537,7 @@ static void nvmet_rdma_send_done(struct ib_cq *cq, struct ib_wc *wc)
 		     wc->status != IB_WC_WR_FLUSH_ERR)) {
 		pr_err("SEND for CQE 0x%p failed with status %s (%d).\n",
 			wc->wr_cqe, ib_wc_status_msg(wc->status), wc->status);
-		nvmet_rdma_error_comp(rsp->queue);
+		nvmet_rdma_error_comp(queue);
 	}
 }
Signature · 1 line
1.8.3.1
author
#1 lore raw

PATCH 2/2 nvme: Remove unused function

Signed-off-by Israel Rukshin <israelr at mellanox.com>
Reviewed-by Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/host/nvme.h | 9 ---------
 1 file changed, 9 deletions(-)
drivers/nvme/host/nvme.h +0 -9
index cee79cb..5486d4c 100644
@@ -364,15 +364,6 @@ static inline void nvme_fault_inject_fini(struct nvme_ns *ns) {}
 static inline void nvme_should_fail(struct request *req) {}
 #endif
 
-static inline bool nvme_ctrl_ready(struct nvme_ctrl *ctrl)
-{
-	u32 val = 0;
-
-	if (ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &val))
-		return false;
-	return val & NVME_CSTS_RDY;
-}
-
 static inline int nvme_reset_subsystem(struct nvme_ctrl *ctrl)
 {
 	if (!ctrl->subsystem)
Signature · 1 line
1.8.3.1
maintainer
#2 lore raw

PATCH 2/2 nvme: Remove unused function

Looks fine,

Reviewed-by Sagi Grimberg <sagi at grimberg.me>
maintainer
#4 lore raw

PATCH 1/2 nvmet-rdma: Fix response use after free

In reply to Israel Rukshin

drivers/nvme/target/rdma.c static void nvmet_rdma_send_done(struct ib_cq *cq, struct ib_wc *wc) · line 532

Quoted 10 lines
drivers/nvme/target/rdma.c +1 -0
index 3f7971d3..732806a 100644
@@ -529,6 +529,7 @@ static void nvmet_rdma_send_done(struct ib_cq *cq, struct ib_wc *wc)
  {
  	struct nvmet_rdma_rsp *rsp =
  		container_of(wc->wr_cqe, struct nvmet_rdma_rsp, send_cqe);
+	struct nvmet_rdma_queue *queue = rsp->queue;

Didn't you mean:
+ struct nvmet_rdma_queue *queue = cq->cq_context;

I'm not sure what this is solving otherwise...

Quoted 13 lines
drivers/nvme/target/rdma.c +1 -1
  	nvmet_rdma_release_rsp(rsp);
 

@@ -536,7 +537,7 @@ static void nvmet_rdma_send_done(struct ib_cq *cq, struct ib_wc *wc)
  		     wc->status != IB_WC_WR_FLUSH_ERR)) {
  		pr_err("SEND for CQE 0x%p failed with status %s (%d).\n",
  			wc->wr_cqe, ib_wc_status_msg(wc->status), wc->status);
-		nvmet_rdma_error_comp(rsp->queue);
+		nvmet_rdma_error_comp(queue);
  	}
  }
#5 lore raw

PATCH 1/2 nvmet-rdma: Fix response use after free

Quoted 15 lines · Sagi Grimberg
Quoted 11 lines
drivers/nvme/target/rdma.c +0 -0
index 3f7971d3..732806a 100644
@@ -529,6 +529,7 @@ static void nvmet_rdma_send_done(struct ib_cq 
 *cq, struct ib_wc *wc)
 ? {

????? struct nvmet_rdma_rsp *rsp =
????????? container_of(wc->wr_cqe, struct nvmet_rdma_rsp, send_cqe); +??? struct nvmet_rdma_queue *queue = rsp->queue;

Didn't you mean:
+??? struct nvmet_rdma_queue *queue = cq->cq_context;

Quoted 1 line

I'm not sure what this is solving otherwise...

it doesn't matter where you take the queue pointer from.

the important thing is to take it before you run nvmet_rdma_release_rsp(rsp) because after your commit that allocate the rsp dynamically

you free it too:

if (rsp->allocated) {
??????????????? kfree(rsp);
??????????????? return;
??????? }

so you might get use after free ... (nvmet_rdma_error_comp(rsp->queue); )

Quoted 12 lines
Quoted 12 lines

? ????? nvmet_rdma_release_rsp(rsp);
? @@ -536,7 +537,7 @@ static void nvmet_rdma_send_done(struct ib_cq *cq, struct ib_wc *wc)
?????????????? wc->status != IB_WC_WR_FLUSH_ERR)) { ????????? pr_err("SEND for CQE 0x%p failed with status %s (%d).\n", ????????????? wc->wr_cqe, ib_wc_status_msg(wc->status), wc->status); -??????? nvmet_rdma_error_comp(rsp->queue);
+??????? nvmet_rdma_error_comp(queue);
????? }
? }

maintainer
#6 lore raw

PATCH 1/2 nvmet-rdma: Fix response use after free

Quoted 23 lines
Quoted 15 lines
Quoted 11 lines
drivers/nvme/target/rdma.c +0 -0
index 3f7971d3..732806a 100644
@@ -529,6 +529,7 @@ static void nvmet_rdma_send_done(struct ib_cq 
 *cq, struct ib_wc *wc)
 ? {

????? struct nvmet_rdma_rsp *rsp =
????????? container_of(wc->wr_cqe, struct nvmet_rdma_rsp, send_cqe); +??? struct nvmet_rdma_queue *queue = rsp->queue;

Didn't you mean:
+??? struct nvmet_rdma_queue *queue = cq->cq_context;

Quoted 1 line

I'm not sure what this is solving otherwise...

it doesn't matter where you take the queue pointer from.

the important thing is to take it before you run nvmet_rdma_release_rsp(rsp) because after your commit that allocate the rsp dynamically

Quoted 8 lines

you free it too:

if (rsp->allocated) {
??????????????? kfree(rsp);
??????????????? return;
??????? }

so you might get use after free ... (nvmet_rdma_error_comp(rsp->queue); )

Yes you're right, now I get what this is doing. But perhaps for consistency lets use the cq_context across all the completion handlers.