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>

diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index 5552fa7426bc9b317da906dec2e395bbf854ae78..5f7151d90344a9509d7275e049b2320e7612430e 100644 (file)
--- 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 040f05fde4d6f69bf00a71eeee1a64c659ab5f67..8a29e3fd01955b5a659e3b543b382541defda083 100644 (file)
--- 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 f1405d335a968d093aadafee06ab4be105970f88..941c84bf1065f406cfbc3eeb5f4b09e4fdcd58ec 100644 (file)
--- 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 *,