[stgt] [PATCH 02/15] Fix use-after-free for task management requests

[FUJITA Tomonori]

On Fri, 12 Jun 2009 11:35:49 +0900
FUJITA Tomonori <fujita.tomonori at lab.ntt.co.jp> wrote:

> On Tue, 09 Jun 2009 18:21:53 +0200
> Arne Redlich <arne.redlich at googlemail.com> wrote:
> 
> > If a task management request for a command that is already processed is
> > received, the tm req will wait for the command to finish. If meanwhile the
> > connection is closed (as a means of error recovery), the tm req might be
> > freed prematurely, which will lead to a use-after-free upon completion of the
> > task.
> > 
> > Signed-off-by: Arne Redlich <arne.redlich at googlemail.com>
> > ---
> >  usr/iscsi/iscsid.c |   18 +++++++++++++++---
> >  usr/iscsi/iscsid.h |    1 +
> >  usr/target.c       |   19 +++++++++++++------
> >  usr/tgtd.h         |   14 +++++++++++---
> >  4 files changed, 40 insertions(+), 12 deletions(-)
> 
> I think that this patch is correct however, we don't need
> task->conn->state checking in iscsi_tm_done() to free tm req (as
> iscsi_scsi_cmd_done does)?

Sorry for the delay, I've merged this with some cleanups.

The following patch was merged:

=
From: Arne Redlich <arne.redlich at googlemail.com>
Subject: [PATCH] fix use-after-free for task management requests

If a task management request for a command that is already processed
is received, the tm req will wait for the command to finish. If
meanwhile the connection is closed (as a means of error recovery), the
tm req might be freed prematurely, which will lead to a use-after-free
upon completion of th\ e task.

Signed-off-by: Arne Redlich <arne.redlich at googlemail.com>
Signed-off-by: FUJITA Tomonori <fujita.tomonori at lab.ntt.co.jp>
---
 usr/iscsi/iscsid.c |   20 +++++++++++++++++---
 usr/target.c       |   22 +++++++++++++++++-----
 usr/tgtd.h         |   14 +++++++++++---
 3 files changed, 45 insertions(+), 11 deletions(-)

diff --git a/usr/iscsi/iscsid.c b/usr/iscsi/iscsid.c
index b952c08..206a67d 100644
--- a/usr/iscsi/iscsid.c
+++ b/usr/iscsi/iscsid.c
@@ -1332,9 +1332,23 @@ static int iscsi_tm_execute(struct iscsi_task *task)
 
 	if (err)
 		task->result = err;
-	else
-		target_mgmt_request(conn->session->target->tid, conn->session->tsih,
-				    (unsigned long) task, fn, req->lun, req->itt, 0);
+	else {
+		int ret;
+		ret = target_mgmt_request(conn->session->target->tid,
+					  conn->session->tsih,
+					  (unsigned long)task, fn, req->lun,
+					  req->itt, 0);
+		set_task_in_scsi(task);
+		switch (ret) {
+		case MGMT_REQ_QUEUED:
+			break;
+		case MGMT_REQ_FAILED:
+		case MGMT_REQ_DONE:
+			clear_task_in_scsi(task);
+			break;
+		}
+	}
+
 	return err;
 }
 
diff --git a/usr/target.c b/usr/target.c
index 08c0dca..9d979bf 100644
--- a/usr/target.c
+++ b/usr/target.c
@@ -1005,8 +1005,10 @@ static int abort_task_set(struct mgmt_req *mreq, struct target* target,
 	return count;
 }
 
-void target_mgmt_request(int tid, uint64_t itn_id, uint64_t req_id,
-			 int function, uint8_t *lun_buf, uint64_t tag, int host_no)
+enum mgmt_req_result target_mgmt_request(int tid, uint64_t itn_id,
+					 uint64_t req_id, int function,
+					 uint8_t *lun_buf, uint64_t tag,
+					 int host_no)
 {
 	struct target *target;
 	struct mgmt_req *mreq;
@@ -1019,12 +1021,15 @@ void target_mgmt_request(int tid, uint64_t itn_id, uint64_t req_id,
 	target = target_lookup(tid);
 	if (!target) {
 		eprintf("invalid tid %d\n", tid);
-		return;
+		return MGMT_REQ_FAILED;
 	}
 
 	mreq = zalloc(sizeof(*mreq));
-	if (!mreq)
-		return;
+	if (!mreq) {
+		eprintf("failed to allocate mgmt_req\n");
+		return MGMT_REQ_FAILED;
+	}
+
 	mreq->mid = req_id;
 	mreq->itn_id = itn_id;
 	mreq->function = function;
@@ -1096,6 +1101,13 @@ void target_mgmt_request(int tid, uint64_t itn_id, uint64_t req_id,
 		tgt_drivers[target->lid]->mgmt_end_notify(mreq);
 		free(mreq);
 	}
+
+	if (err)
+		return err;
+	else if (send)
+		return MGMT_REQ_DONE;
+
+	return MGMT_REQ_QUEUED;
 }
 
 struct account_entry {
diff --git a/usr/tgtd.h b/usr/tgtd.h
index 12a20dc..303627e 100644
--- a/usr/tgtd.h
+++ b/usr/tgtd.h
@@ -169,6 +169,12 @@ struct mgmt_req {
 	uint64_t itn_id;
 };
 
+enum mgmt_req_result {
+	MGMT_REQ_FAILED = -1,
+	MGMT_REQ_DONE,
+	MGMT_REQ_QUEUED,
+};
+
 #ifdef USE_KERNEL
 extern int kreq_init(void);
 #else
@@ -224,9 +230,11 @@ extern int tgt_event_modify(int fd, int events);
 extern int target_cmd_queue(int tid, struct scsi_cmd *cmd);
 extern void target_cmd_done(struct scsi_cmd *cmd);
 struct scsi_cmd *target_cmd_lookup(int tid, uint64_t itn_id, uint64_t tag);
-extern void target_mgmt_request(int tid, uint64_t itn_id, uint64_t req_id,
-				int function, uint8_t *lun, uint64_t tag,
-				int host_no);
+
+extern enum mgmt_req_result target_mgmt_request(int tid, uint64_t itn_id,
+						uint64_t req_id, int function,
+						uint8_t *lun, uint64_t tag,
+						int host_no);
 
 extern void target_cmd_io_done(struct scsi_cmd *cmd, int result);
 extern int ua_sense_del(struct scsi_cmd *cmd, int del);
-- 
1.6.0.6


--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html