linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 3.6 0/3] Fix use-after-free in PSCSI
@ 2012-09-05 15:09 Paolo Bonzini
  2012-09-05 15:09 ` [PATCH 3.6 1/3] target: move transport_get_sense_data Paolo Bonzini
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Paolo Bonzini @ 2012-09-05 15:09 UTC (permalink / raw)
  To: linux-kernel; +Cc: target-devel, nab

Hi,

this series fixes the bug I reported with wrong sense data.  The
memory corruption is caused by using the sense data after freeing
it.  The series corrects it by moving the copy of the sense data
earlier, to the transport_complete callback.

Please review and be kind on my first lio patches! :)

Paolo

Paolo Bonzini (3):
  target: move transport_get_sense_data
  target: simplify code around transport_get_sense_data
  target: fix use-after-free with PSCSI sense data

 drivers/target/target_core_pscsi.c     |   21 ++----
 drivers/target/target_core_transport.c |  117 ++++++++++++--------------------
 include/target/target_core_backend.h   |    4 +-
 3 files changed, 53 insertions(+), 89 deletions(-)


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 3.6 1/3] target: move transport_get_sense_data
  2012-09-05 15:09 [PATCH 3.6 0/3] Fix use-after-free in PSCSI Paolo Bonzini
@ 2012-09-05 15:09 ` Paolo Bonzini
  2012-09-05 15:09 ` [PATCH 3.6 2/3] target: simplify code around transport_get_sense_data Paolo Bonzini
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Paolo Bonzini @ 2012-09-05 15:09 UTC (permalink / raw)
  To: linux-kernel; +Cc: target-devel, nab

We will be calling it from transport_complete_cmd, avoid forward
declarations.  No semantic change.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 drivers/target/target_core_transport.c |  110 ++++++++++++++++----------------
 1 files changed, 55 insertions(+), 55 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 4de3186..9f5ff8d 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -567,6 +567,61 @@ static void target_complete_failure_work(struct work_struct *work)
 	transport_generic_request_failure(cmd);
 }
 
+/*
+ * Used to obtain Sense Data from underlying Linux/SCSI struct scsi_cmnd
+ */
+static int transport_get_sense_data(struct se_cmd *cmd)
+{
+	unsigned char *buffer = cmd->sense_buffer, *sense_buffer = NULL;
+	struct se_device *dev = cmd->se_dev;
+	unsigned long flags;
+	u32 offset = 0;
+
+	WARN_ON(!cmd->se_lun);
+
+	if (!dev)
+		return 0;
+
+	spin_lock_irqsave(&cmd->t_state_lock, flags);
+	if (cmd->se_cmd_flags & SCF_SENT_CHECK_CONDITION) {
+		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+		return 0;
+	}
+
+	if (!(cmd->se_cmd_flags & SCF_TRANSPORT_TASK_SENSE))
+		goto out;
+
+	if (!dev->transport->get_sense_buffer) {
+		pr_err("dev->transport->get_sense_buffer is NULL\n");
+		goto out;
+	}
+
+	sense_buffer = dev->transport->get_sense_buffer(cmd);
+	if (!sense_buffer) {
+		pr_err("ITT 0x%08x cmd %p: Unable to locate"
+			" sense buffer for task with sense\n",
+			cmd->se_tfo->get_task_tag(cmd), cmd);
+		goto out;
+	}
+
+	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+
+	offset = cmd->se_tfo->set_fabric_sense_len(cmd, TRANSPORT_SENSE_BUFFER);
+
+	memcpy(&buffer[offset], sense_buffer, TRANSPORT_SENSE_BUFFER);
+
+	/* Automatically padded */
+	cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER + offset;
+
+	pr_debug("HBA_[%u]_PLUG[%s]: Set SAM STATUS: 0x%02x and sense\n",
+		dev->se_hba->hba_id, dev->transport->name, cmd->scsi_status);
+	return 0;
+
+out:
+	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+	return -1;
+}
+
 void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status)
 {
 	struct se_device *dev = cmd->se_dev;
@@ -1816,61 +1871,6 @@ execute:
 EXPORT_SYMBOL(target_execute_cmd);
 
 /*
- * Used to obtain Sense Data from underlying Linux/SCSI struct scsi_cmnd
- */
-static int transport_get_sense_data(struct se_cmd *cmd)
-{
-	unsigned char *buffer = cmd->sense_buffer, *sense_buffer = NULL;
-	struct se_device *dev = cmd->se_dev;
-	unsigned long flags;
-	u32 offset = 0;
-
-	WARN_ON(!cmd->se_lun);
-
-	if (!dev)
-		return 0;
-
-	spin_lock_irqsave(&cmd->t_state_lock, flags);
-	if (cmd->se_cmd_flags & SCF_SENT_CHECK_CONDITION) {
-		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
-		return 0;
-	}
-
-	if (!(cmd->se_cmd_flags & SCF_TRANSPORT_TASK_SENSE))
-		goto out;
-
-	if (!dev->transport->get_sense_buffer) {
-		pr_err("dev->transport->get_sense_buffer is NULL\n");
-		goto out;
-	}
-
-	sense_buffer = dev->transport->get_sense_buffer(cmd);
-	if (!sense_buffer) {
-		pr_err("ITT 0x%08x cmd %p: Unable to locate"
-			" sense buffer for task with sense\n",
-			cmd->se_tfo->get_task_tag(cmd), cmd);
-		goto out;
-	}
-
-	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
-
-	offset = cmd->se_tfo->set_fabric_sense_len(cmd, TRANSPORT_SENSE_BUFFER);
-
-	memcpy(&buffer[offset], sense_buffer, TRANSPORT_SENSE_BUFFER);
-
-	/* Automatically padded */
-	cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER + offset;
-
-	pr_debug("HBA_[%u]_PLUG[%s]: Set SAM STATUS: 0x%02x and sense\n",
-		dev->se_hba->hba_id, dev->transport->name, cmd->scsi_status);
-	return 0;
-
-out:
-	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
-	return -1;
-}
-
-/*
  * Process all commands up to the last received ORDERED task attribute which
  * requires another blocking boundary
  */
-- 
1.7.1



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 3.6 2/3] target: simplify code around transport_get_sense_data
  2012-09-05 15:09 [PATCH 3.6 0/3] Fix use-after-free in PSCSI Paolo Bonzini
  2012-09-05 15:09 ` [PATCH 3.6 1/3] target: move transport_get_sense_data Paolo Bonzini
@ 2012-09-05 15:09 ` Paolo Bonzini
  2012-09-05 15:09 ` [PATCH 3.6 3/3] target: fix use-after-free with PSCSI sense data Paolo Bonzini
  2012-09-06  0:41 ` [PATCH 3.6 0/3] Fix use-after-free in PSCSI Nicholas A. Bellinger
  3 siblings, 0 replies; 5+ messages in thread
From: Paolo Bonzini @ 2012-09-05 15:09 UTC (permalink / raw)
  To: linux-kernel; +Cc: target-devel, nab

The error conditions in transport_get_sense_data are superfluous
and complicate the code unnecessarily:

* SCF_TRANSPORT_TASK_SENSE is checked in the caller;

* it's simply part of the invariants of dev->transport->get_sense_buffer
  that it must be there if transport_complete ever returns 1, and that
  it must not return NULL.  Besides, the entire callback will disappear
  with the next patch.

* similarly in the caller we can expect that sense data is only sent
  for non-zero cmd->scsi_status.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 drivers/target/target_core_transport.c |   51 +++++++++-----------------------
 1 files changed, 14 insertions(+), 37 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 9f5ff8d..ea01807 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -570,7 +570,7 @@ static void target_complete_failure_work(struct work_struct *work)
 /*
  * Used to obtain Sense Data from underlying Linux/SCSI struct scsi_cmnd
  */
-static int transport_get_sense_data(struct se_cmd *cmd)
+static void transport_get_sense_data(struct se_cmd *cmd)
 {
 	unsigned char *buffer = cmd->sense_buffer, *sense_buffer = NULL;
 	struct se_device *dev = cmd->se_dev;
@@ -580,30 +580,15 @@ static int transport_get_sense_data(struct se_cmd *cmd)
 	WARN_ON(!cmd->se_lun);
 
 	if (!dev)
-		return 0;
+		return;
 
 	spin_lock_irqsave(&cmd->t_state_lock, flags);
 	if (cmd->se_cmd_flags & SCF_SENT_CHECK_CONDITION) {
 		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
-		return 0;
-	}
-
-	if (!(cmd->se_cmd_flags & SCF_TRANSPORT_TASK_SENSE))
-		goto out;
-
-	if (!dev->transport->get_sense_buffer) {
-		pr_err("dev->transport->get_sense_buffer is NULL\n");
-		goto out;
+		return;
 	}
 
 	sense_buffer = dev->transport->get_sense_buffer(cmd);
-	if (!sense_buffer) {
-		pr_err("ITT 0x%08x cmd %p: Unable to locate"
-			" sense buffer for task with sense\n",
-			cmd->se_tfo->get_task_tag(cmd), cmd);
-		goto out;
-	}
-
 	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 
 	offset = cmd->se_tfo->set_fabric_sense_len(cmd, TRANSPORT_SENSE_BUFFER);
@@ -615,11 +600,6 @@ static int transport_get_sense_data(struct se_cmd *cmd)
 
 	pr_debug("HBA_[%u]_PLUG[%s]: Set SAM STATUS: 0x%02x and sense\n",
 		dev->se_hba->hba_id, dev->transport->name, cmd->scsi_status);
-	return 0;
-
-out:
-	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
-	return -1;
 }
 
 void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status)
@@ -1985,7 +1965,7 @@ static void transport_handle_queue_full(
 static void target_complete_ok_work(struct work_struct *work)
 {
 	struct se_cmd *cmd = container_of(work, struct se_cmd, work);
-	int reason = 0, ret;
+	int ret;
 
 	/*
 	 * Check if we need to move delayed/dormant tasks from cmds on the
@@ -2006,19 +1986,16 @@ static void target_complete_ok_work(struct work_struct *work)
 	 * the struct se_cmd in question.
 	 */
 	if (cmd->se_cmd_flags & SCF_TRANSPORT_TASK_SENSE) {
-		if (transport_get_sense_data(cmd) < 0)
-			reason = TCM_NON_EXISTENT_LUN;
-
-		if (cmd->scsi_status) {
-			ret = transport_send_check_condition_and_sense(
-					cmd, reason, 1);
-			if (ret == -EAGAIN || ret == -ENOMEM)
-				goto queue_full;
+		WARN_ON(!cmd->scsi_status);
+		transport_get_sense_data(cmd);
+		ret = transport_send_check_condition_and_sense(
+					cmd, 0, 1);
+		if (ret == -EAGAIN || ret == -ENOMEM)
+			goto queue_full;
 
-			transport_lun_remove_cmd(cmd);
-			transport_cmd_check_stop_to_fabric(cmd);
-			return;
-		}
+		transport_lun_remove_cmd(cmd);
+		transport_cmd_check_stop_to_fabric(cmd);
+		return;
 	}
 	/*
 	 * Check for a callback, used by amongst other things
-- 
1.7.1



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 3.6 3/3] target: fix use-after-free with PSCSI sense data
  2012-09-05 15:09 [PATCH 3.6 0/3] Fix use-after-free in PSCSI Paolo Bonzini
  2012-09-05 15:09 ` [PATCH 3.6 1/3] target: move transport_get_sense_data Paolo Bonzini
  2012-09-05 15:09 ` [PATCH 3.6 2/3] target: simplify code around transport_get_sense_data Paolo Bonzini
@ 2012-09-05 15:09 ` Paolo Bonzini
  2012-09-06  0:41 ` [PATCH 3.6 0/3] Fix use-after-free in PSCSI Nicholas A. Bellinger
  3 siblings, 0 replies; 5+ messages in thread
From: Paolo Bonzini @ 2012-09-05 15:09 UTC (permalink / raw)
  To: linux-kernel; +Cc: target-devel, nab

The pointer to the sense buffer is fetched by transport_get_sense_data,
but this is called by target_complete_ok_work long after pscsi_req_done
has freed the struct that contains it.

Pass instead the fabric's sense buffer to transport_complete,
and copy the data to it directly in transport_complete.  Setting
SCF_TRANSPORT_TASK_SENSE also becomes a duty of transport_complete.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 drivers/target/target_core_pscsi.c     |   21 ++++++------------
 drivers/target/target_core_transport.c |   36 ++++++++++++-------------------
 include/target/target_core_backend.h   |    4 ++-
 3 files changed, 24 insertions(+), 37 deletions(-)

diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index 5552fa7..5f7151d 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -667,7 +667,8 @@ static void pscsi_free_device(void *p)
 	kfree(pdv);
 }
 
-static int pscsi_transport_complete(struct se_cmd *cmd, struct scatterlist *sg)
+static void pscsi_transport_complete(struct se_cmd *cmd, struct scatterlist *sg,
+				     unsigned char *sense_buffer)
 {
 	struct pscsi_dev_virt *pdv = cmd->se_dev->dev_ptr;
 	struct scsi_device *sd = pdv->pdv_sd;
@@ -679,7 +680,7 @@ static int pscsi_transport_complete(struct se_cmd *cmd, struct scatterlist *sg)
 	 * not been allocated because TCM is handling the emulation directly.
 	 */
 	if (!pt)
-		return 0;
+		return;
 
 	cdb = &pt->pscsi_cdb[0];
 	result = pt->pscsi_result;
@@ -750,10 +751,10 @@ after_mode_sense:
 	}
 after_mode_select:
 
-	if (status_byte(result) & CHECK_CONDITION)
-		return 1;
-
-	return 0;
+	if (sense_buffer && (status_byte(result) & CHECK_CONDITION)) {
+		memcpy(sense_buffer, pt->pscsi_sense, TRANSPORT_SENSE_BUFFER);
+		cmd->se_cmd_flags |= SCF_TRANSPORT_TASK_SENSE;
+	}
 }
 
 enum {
@@ -1184,13 +1185,6 @@ fail:
 	return -ENOMEM;
 }
 
-static unsigned char *pscsi_get_sense_buffer(struct se_cmd *cmd)
-{
-	struct pscsi_plugin_task *pt = cmd->priv;
-
-	return pt->pscsi_sense;
-}
-
 /*	pscsi_get_device_rev():
  *
  *
@@ -1273,7 +1267,6 @@ static struct se_subsystem_api pscsi_template = {
 	.check_configfs_dev_params = pscsi_check_configfs_dev_params,
 	.set_configfs_dev_params = pscsi_set_configfs_dev_params,
 	.show_configfs_dev_params = pscsi_show_configfs_dev_params,
-	.get_sense_buffer	= pscsi_get_sense_buffer,
 	.get_device_rev		= pscsi_get_device_rev,
 	.get_device_type	= pscsi_get_device_type,
 	.get_blocks		= pscsi_get_blocks,
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index ea01807..09028af 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -568,38 +568,31 @@ static void target_complete_failure_work(struct work_struct *work)
 }
 
 /*
- * Used to obtain Sense Data from underlying Linux/SCSI struct scsi_cmnd
+ * Used when asking transport to copy Sense Data from the underlying
+ * Linux/SCSI struct scsi_cmnd
  */
-static void transport_get_sense_data(struct se_cmd *cmd)
+static unsigned char *transport_get_sense_buffer(struct se_cmd *cmd)
 {
-	unsigned char *buffer = cmd->sense_buffer, *sense_buffer = NULL;
+	unsigned char *buffer = cmd->sense_buffer;
 	struct se_device *dev = cmd->se_dev;
-	unsigned long flags;
 	u32 offset = 0;
 
 	WARN_ON(!cmd->se_lun);
 
 	if (!dev)
-		return;
-
-	spin_lock_irqsave(&cmd->t_state_lock, flags);
-	if (cmd->se_cmd_flags & SCF_SENT_CHECK_CONDITION) {
-		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
-		return;
-	}
+		return NULL;
 
-	sense_buffer = dev->transport->get_sense_buffer(cmd);
-	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+	if (cmd->se_cmd_flags & SCF_SENT_CHECK_CONDITION)
+		return NULL;
 
 	offset = cmd->se_tfo->set_fabric_sense_len(cmd, TRANSPORT_SENSE_BUFFER);
 
-	memcpy(&buffer[offset], sense_buffer, TRANSPORT_SENSE_BUFFER);
-
 	/* Automatically padded */
 	cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER + offset;
 
-	pr_debug("HBA_[%u]_PLUG[%s]: Set SAM STATUS: 0x%02x and sense\n",
+	pr_debug("HBA_[%u]_PLUG[%s]: Requesting sense for SAM STATUS: 0x%02x\n",
 		dev->se_hba->hba_id, dev->transport->name, cmd->scsi_status);
+	return &buffer[offset];
 }
 
 void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status)
@@ -615,11 +608,11 @@ void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status)
 	cmd->transport_state &= ~CMD_T_BUSY;
 
 	if (dev && dev->transport->transport_complete) {
-		if (dev->transport->transport_complete(cmd,
-				cmd->t_data_sg) != 0) {
-			cmd->se_cmd_flags |= SCF_TRANSPORT_TASK_SENSE;
+		dev->transport->transport_complete(cmd,
+				cmd->t_data_sg,
+				transport_get_sense_buffer(cmd));
+		if (cmd->se_cmd_flags & SCF_TRANSPORT_TASK_SENSE)
 			success = 1;
-		}
 	}
 
 	/*
@@ -1982,12 +1975,11 @@ static void target_complete_ok_work(struct work_struct *work)
 		schedule_work(&cmd->se_dev->qf_work_queue);
 
 	/*
-	 * Check if we need to retrieve a sense buffer from
+	 * Check if we need to send a sense buffer from
 	 * the struct se_cmd in question.
 	 */
 	if (cmd->se_cmd_flags & SCF_TRANSPORT_TASK_SENSE) {
 		WARN_ON(!cmd->scsi_status);
-		transport_get_sense_data(cmd);
 		ret = transport_send_check_condition_and_sense(
 					cmd, 0, 1);
 		if (ret == -EAGAIN || ret == -ENOMEM)
diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
index f1405d3..8d591e4 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -23,7 +23,9 @@ struct se_subsystem_api {
 	struct se_device *(*create_virtdevice)(struct se_hba *,
 				struct se_subsystem_dev *, void *);
 	void (*free_device)(void *);
-	int (*transport_complete)(struct se_cmd *cmd, struct scatterlist *);
+	void (*transport_complete)(struct se_cmd *cmd,
+				   struct scatterlist *,
+				   unsigned char *);
 
 	int (*parse_cdb)(struct se_cmd *cmd);
 	ssize_t (*check_configfs_dev_params)(struct se_hba *,
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH 3.6 0/3] Fix use-after-free in PSCSI
  2012-09-05 15:09 [PATCH 3.6 0/3] Fix use-after-free in PSCSI Paolo Bonzini
                   ` (2 preceding siblings ...)
  2012-09-05 15:09 ` [PATCH 3.6 3/3] target: fix use-after-free with PSCSI sense data Paolo Bonzini
@ 2012-09-06  0:41 ` Nicholas A. Bellinger
  3 siblings, 0 replies; 5+ messages in thread
From: Nicholas A. Bellinger @ 2012-09-06  0:41 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, target-devel

On Wed, 2012-09-05 at 17:09 +0200, Paolo Bonzini wrote:
> Hi,
> 
> this series fixes the bug I reported with wrong sense data.  The
> memory corruption is caused by using the sense data after freeing
> it.  The series corrects it by moving the copy of the sense data
> earlier, to the transport_complete callback.
> 
> Please review and be kind on my first lio patches! :)
> 
> Paolo
> 
> Paolo Bonzini (3):
>   target: move transport_get_sense_data
>   target: simplify code around transport_get_sense_data
>   target: fix use-after-free with PSCSI sense data
> 
>  drivers/target/target_core_pscsi.c     |   21 ++----
>  drivers/target/target_core_transport.c |  117 ++++++++++++--------------------
>  include/target/target_core_backend.h   |    4 +-
>  3 files changed, 53 insertions(+), 89 deletions(-)
> 

The approach to squash the bug looks easonable to me, and nice work on
the extra cleanups for backend sense w/ TRANSPORT_PLUGIN_PHBA_PDEV !

I've applied this to target-pending/master w/ CC stable, and will plan
to send out the PULL request for -rc5 over the next days.

Nice work Paolo!

--nab


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2012-09-06  0:41 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-05 15:09 [PATCH 3.6 0/3] Fix use-after-free in PSCSI Paolo Bonzini
2012-09-05 15:09 ` [PATCH 3.6 1/3] target: move transport_get_sense_data Paolo Bonzini
2012-09-05 15:09 ` [PATCH 3.6 2/3] target: simplify code around transport_get_sense_data Paolo Bonzini
2012-09-05 15:09 ` [PATCH 3.6 3/3] target: fix use-after-free with PSCSI sense data Paolo Bonzini
2012-09-06  0:41 ` [PATCH 3.6 0/3] Fix use-after-free in PSCSI Nicholas A. Bellinger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).