target: fix use-after-free with PSCSI sense data

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>
Cc: stable@vger.kernel.org
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>

3 files changed, 24 insertions, 37 deletions

diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index 5552fa7426bc..5f7151d90344 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 040f05fde4d6..8a29e3fd0195 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;
-                }
         }
 
         /*
@@ -1987,12 +1980,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 f1405d335a96..941c84bf1065 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 *,