* [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