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

[FUJITA Tomonori]

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)?


> diff --git a/usr/iscsi/iscsid.c b/usr/iscsi/iscsid.c
> index 9252f4a..41e34de 100644
> --- a/usr/iscsi/iscsid.c
> +++ b/usr/iscsi/iscsid.c
> @@ -1325,9 +1325,21 @@ 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 {
> +		set_task_in_scsi(task);
> +		switch (target_mgmt_request(conn->session->target->tid,
> +					    conn->session->tsih,
> +					    (unsigned long) task, fn, req->lun,
> +					    req->itt, 0)) {
> +		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/iscsi/iscsid.h b/usr/iscsi/iscsid.h
> index f90e0e9..7897c41 100644
> --- a/usr/iscsi/iscsid.h
> +++ b/usr/iscsi/iscsid.h
> @@ -252,6 +252,7 @@ enum task_flags {
>  #define task_pending(t)		((t)->flags & (1 << TASK_pending))
>  
>  #define set_task_in_scsi(t)     ((t)->flags |= (1 << TASK_in_scsi))
> +#define clear_task_in_scsi(t)	((t)->flags &= ~(1 << TASK_in_scsi))
>  #define task_in_scsi(t)		((t)->flags & (1 << TASK_in_scsi))
>  
>  extern int lld_index;
> diff --git a/usr/target.c b/usr/target.c
> index dc30c87..b7ea99d 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;
> @@ -1095,7 +1100,9 @@ void target_mgmt_request(int tid, uint64_t itn_id, uint64_t req_id,
>  		mreq->result = err;
>  		tgt_drivers[target->lid]->mgmt_end_notify(mreq);
>  		free(mreq);
> -	}
> +		return err ? MGMT_REQ_FAILED : MGMT_REQ_DONE;
> +	} else
> +		return err ? MGMT_REQ_FAILED : 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.4
> 
> 
> 
> --
> 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
--
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