From 4c054ba63ad47ef244cfcfa1cea38134620a5bae Mon Sep 17 00:00:00 2001 From: Nicholas Bellinger Date: Thu, 16 Aug 2012 15:33:10 -0700 Subject: target: Fix ->data_length re-assignment bug with SCSI overflow This patch fixes a long-standing bug with SCSI overflow handling where se_cmd->data_length was incorrectly being re-assigned to the larger CDB extracted allocation length, resulting in a number of fabric level errors that would end up causing a session reset in most cases. So instead now: - Only re-assign se_cmd->data_length durining UNDERFLOW (to use the smaller value) - Use existing se_cmd->data_length for OVERFLOW (to use the smaller value) This fix has been tested with the following CDB to generate an SCSI overflow: sg_raw -r512 /dev/sdc 28 0 0 0 0 0 0 0 9 0 Tested using iscsi-target, tcm_qla2xxx, loopback and tcm_vhost fabric ports. Here is a bit more detail on each case: - iscsi-target: Bug with open-iscsi with overflow, sg_raw returns -3584 bytes of data. - tcm_qla2xxx: Working as expected, returnins 512 bytes of data - loopback: sg_raw returns CHECK_CONDITION, from overflow rejection in transport_generic_map_mem_to_cmd() - tcm_vhost: Same as loopback Reported-by: Roland Dreier Cc: Roland Dreier Cc: Christoph Hellwig Cc: Boaz Harrosh Cc: Signed-off-by: Nicholas Bellinger diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 4de3186..3425098 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -1181,15 +1181,20 @@ int target_cmd_size_check(struct se_cmd *cmd, unsigned int size) /* Returns CHECK_CONDITION + INVALID_CDB_FIELD */ goto out_invalid_cdb_field; } - + /* + * For the overflow case keep the existing fabric provided + * ->data_length. Otherwise for the underflow case, reset + * ->data_length to the smaller SCSI expected data transfer + * length. + */ if (size > cmd->data_length) { cmd->se_cmd_flags |= SCF_OVERFLOW_BIT; cmd->residual_count = (size - cmd->data_length); } else { cmd->se_cmd_flags |= SCF_UNDERFLOW_BIT; cmd->residual_count = (cmd->data_length - size); + cmd->data_length = size; } - cmd->data_length = size; } return 0; -- cgit v0.10.2 From 13b5533a38b1eec9d61a07711dc075f03ae47a36 Mon Sep 17 00:00:00 2001 From: Benjamin Wang Date: Sun, 26 Aug 2012 18:04:10 +0800 Subject: target: Check idr_get_new return value in iscsi_login_zero_tsih_s1 This patch updates iscsi_login_zero_tsih_s1() usage for generating iscsi_session->session_index to properly check the return value from idr_get_new(), and reject the iSCSI login attempt with exception status ISCSI_LOGIN_STATUS_NO_RESOURCES in the event of a failure. Signed-off-by: Benjamin Wang Signed-off-by: Nicholas Bellinger diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c index 0694d9b..6aba439 100644 --- a/drivers/target/iscsi/iscsi_target_login.c +++ b/drivers/target/iscsi/iscsi_target_login.c @@ -221,6 +221,7 @@ static int iscsi_login_zero_tsih_s1( { struct iscsi_session *sess = NULL; struct iscsi_login_req *pdu = (struct iscsi_login_req *)buf; + int ret; sess = kzalloc(sizeof(struct iscsi_session), GFP_KERNEL); if (!sess) { @@ -257,9 +258,17 @@ static int iscsi_login_zero_tsih_s1( return -ENOMEM; } spin_lock(&sess_idr_lock); - idr_get_new(&sess_idr, NULL, &sess->session_index); + ret = idr_get_new(&sess_idr, NULL, &sess->session_index); spin_unlock(&sess_idr_lock); + if (ret < 0) { + pr_err("idr_get_new() for sess_idr failed\n"); + iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR, + ISCSI_LOGIN_STATUS_NO_RESOURCES); + kfree(sess); + return -ENOMEM; + } + sess->creation_time = get_jiffies_64(); spin_lock_init(&sess->session_stats_lock); /* -- cgit v0.10.2 From 6138ed2ab8791d7a9c5ba66cadadd5eaf1fc1dac Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 5 Sep 2012 17:09:13 +0200 Subject: target: move transport_get_sense_data We will be calling it from transport_complete_cmd, avoid forward declarations. No semantic change. Signed-off-by: Paolo Bonzini Cc: stable@vger.kernel.org Signed-off-by: Nicholas Bellinger diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 3425098..d0afdaa 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; @@ -1821,61 +1876,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 */ -- cgit v0.10.2 From 27a2709912ac19c755d34c79fe11994b0bf8082b Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 5 Sep 2012 17:09:14 +0200 Subject: target: simplify code around transport_get_sense_data 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 Cc: stable@vger.kernel.org Signed-off-by: Nicholas Bellinger diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index d0afdaa..040f05f 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) @@ -1990,7 +1970,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 @@ -2011,19 +1991,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 -- cgit v0.10.2 From d5829eac5f7cfff89c6d1cf11717eee97cf030d0 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 5 Sep 2012 17:09:15 +0200 Subject: 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 Cc: stable@vger.kernel.org Signed-off-by: Nicholas Bellinger 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 040f05f..8a29e3f 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 f1405d33..941c84bf 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 *, -- cgit v0.10.2 From 306c11b28d7bb85a7adda741798a2b6b60dd305a Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 7 Sep 2012 17:30:32 +0200 Subject: target: go through normal processing for zero-length PSCSI commands Right now, commands with a zero-size payload are skipped completely. This is wrong; such commands should be passed down to the device and processed normally. For physical backends, this ignores completely things such as START STOP UNIT. For virtual backends, we have a hack in place to clear a unit attention state on a zero-size REQUEST SENSE, but we still do not report errors properly on zero-length commands---out-of-bounds 0-block reads and writes, too small parameter list lengths, etc. This patch fixes this for PSCSI. Uses of transport_kmap_data_sg are guarded with a check for non-zero cmd->data_length; for all other commands a zero length is handled properly in pscsi_execute_cmd. The sole exception will be for now REPORT LUNS, which is handled through the normal SPC emulation. Signed-off-by: Paolo Bonzini Signed-off-by: Nicholas Bellinger diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c index 5f7151d..9d7ce3d 100644 --- a/drivers/target/target_core_pscsi.c +++ b/drivers/target/target_core_pscsi.c @@ -688,11 +688,11 @@ static void pscsi_transport_complete(struct se_cmd *cmd, struct scatterlist *sg, * Hack to make sure that Write-Protect modepage is set if R/O mode is * forced. */ + if (!cmd->se_deve || !cmd->data_length) + goto after_mode_sense; + if (((cdb[0] == MODE_SENSE) || (cdb[0] == MODE_SENSE_10)) && (status_byte(result) << 1) == SAM_STAT_GOOD) { - if (!cmd->se_deve) - goto after_mode_sense; - if (cmd->se_deve->lun_flags & TRANSPORT_LUNFLAGS_READ_ONLY) { unsigned char *buf = transport_kmap_data_sg(cmd); @@ -709,7 +709,7 @@ static void pscsi_transport_complete(struct se_cmd *cmd, struct scatterlist *sg, } after_mode_sense: - if (sd->type != TYPE_TAPE) + if (sd->type != TYPE_TAPE || !cmd->data_length) goto after_mode_select; /* diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 8a29e3f..7ddb0c3 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -2300,7 +2300,9 @@ int transport_generic_new_cmd(struct se_cmd *cmd) * into the fabric for data transfers, go ahead and complete it right * away. */ - if (!cmd->data_length) { + if (!cmd->data_length && + (cmd->se_dev->transport->transport_type != TRANSPORT_PLUGIN_PHBA_PDEV || + cmd->t_task_cdb[0] == REPORT_LUNS) { spin_lock_irq(&cmd->t_state_lock); cmd->t_state = TRANSPORT_COMPLETE; cmd->transport_state |= CMD_T_ACTIVE; -- cgit v0.10.2 From 0d7f1299ca5540b9a63ab6e8bf0e89ea963eb6af Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 7 Sep 2012 17:30:33 +0200 Subject: target: report too-small parameter lists everywhere Several places were not checking that the parameter list length was large enough, and thus accessing invalid memory. Zero-length parameter lists are just a special case of this. Signed-off-by: Paolo Bonzini Signed-off-by: Nicholas Bellinger diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c index 9179997..41641ba 100644 --- a/drivers/target/target_core_alua.c +++ b/drivers/target/target_core_alua.c @@ -218,6 +218,13 @@ int target_emulate_set_target_port_groups(struct se_cmd *cmd) cmd->scsi_sense_reason = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; return -EINVAL; } + if (cmd->data_length < 4) { + pr_warn("SET TARGET PORT GROUPS parameter list length %u too" + " small\n", cmd->data_length); + cmd->scsi_sense_reason = TCM_INVALID_PARAMETER_LIST; + return -EINVAL; + } + buf = transport_kmap_data_sg(cmd); /* diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c index 76db75e..9ba4954 100644 --- a/drivers/target/target_core_iblock.c +++ b/drivers/target/target_core_iblock.c @@ -325,17 +325,30 @@ static int iblock_execute_unmap(struct se_cmd *cmd) struct iblock_dev *ibd = dev->dev_ptr; unsigned char *buf, *ptr = NULL; sector_t lba; - int size = cmd->data_length; + int size; u32 range; int ret = 0; int dl, bd_dl; + if (cmd->data_length < 8) { + pr_warn("UNMAP parameter list length %u too small\n", + cmd->data_length); + cmd->scsi_sense_reason = TCM_INVALID_PARAMETER_LIST; + return -EINVAL; + } + buf = transport_kmap_data_sg(cmd); dl = get_unaligned_be16(&buf[0]); bd_dl = get_unaligned_be16(&buf[2]); - size = min(size - 8, bd_dl); + size = cmd->data_length - 8; + if (bd_dl > size) + pr_warn("UNMAP parameter list length %u too small, ignoring bd_dl %u\n", + cmd->data_length, bd_dl); + else + size = bd_dl; + if (size / 16 > dev->se_sub_dev->se_dev_attrib.max_unmap_block_desc_count) { cmd->scsi_sense_reason = TCM_INVALID_PARAMETER_LIST; ret = -EINVAL; diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c index 1e94650..956c84c 100644 --- a/drivers/target/target_core_pr.c +++ b/drivers/target/target_core_pr.c @@ -1540,6 +1540,14 @@ static int core_scsi3_decode_spec_i_port( tidh_new->dest_local_nexus = 1; list_add_tail(&tidh_new->dest_list, &tid_dest_list); + if (cmd->data_length < 28) { + pr_warn("SPC-PR: Received PR OUT parameter list" + " length too small: %u\n", cmd->data_length); + cmd->scsi_sense_reason = TCM_INVALID_PARAMETER_LIST; + ret = -EINVAL; + goto out; + } + buf = transport_kmap_data_sg(cmd); /* * For a PERSISTENT RESERVE OUT specify initiator ports payload, -- cgit v0.10.2 From 9b16b9edb4fd0dc86ee0fbe1f7ede580e26e85f4 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 7 Sep 2012 17:30:34 +0200 Subject: target: fail REPORT LUNS with less than 16 bytes of payload SPC says: "The ALLOCATION LENGTH field is defined in 4.3.5.6. The allocation length should be at least 16. Device servers compliant with SPC return CHECK CONDITION status, with the sense key set to ILLEGAL REQUEST, and the additional sense code set to INVALID FIELD IN CDB when the allocation length is less than 16 bytes". Testcase: sg_raw -r8 /dev/sdb a0 00 00 00 00 00 00 00 00 08 00 00 should fail with ILLEGAL REQUEST / INVALID FIELD IN CDB sense does not fail without the patch fails correctly with the patch Signed-off-by: Paolo Bonzini Signed-off-by: Nicholas Bellinger diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c index cf2c66f..9fc9a60 100644 --- a/drivers/target/target_core_device.c +++ b/drivers/target/target_core_device.c @@ -669,6 +669,13 @@ int target_report_luns(struct se_cmd *se_cmd) unsigned char *buf; u32 lun_count = 0, offset = 8, i; + if (se_cmd->data_length < 16) { + pr_warn("REPORT LUNS allocation length %u too small\n", + se_cmd->data_length); + se_cmd->scsi_sense_reason = TCM_INVALID_CDB_FIELD; + return -EINVAL; + } + buf = transport_kmap_data_sg(se_cmd); if (!buf) return -ENOMEM; -- cgit v0.10.2 From 3717ef0c63e90686d959158e9728a13a49229be6 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 7 Sep 2012 17:30:35 +0200 Subject: target: support zero-size allocation lengths in transport_kmap_data_sg In order to support zero-size allocation lengths, do not assert that we have a scatterlist until after checking cmd->data_length. But once we do this, we can have two cases of transport_kmap_data_sg returning NULL: a zero-size allocation length, or an out-of-memory condition. Report the latter using sense codes, so that the SCSI command that triggered it will fail. Signed-off-by: Paolo Bonzini Signed-off-by: Nicholas Bellinger diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 7ddb0c3..4403477 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -2190,7 +2190,6 @@ void *transport_kmap_data_sg(struct se_cmd *cmd) struct page **pages; int i; - BUG_ON(!sg); /* * We need to take into account a possible offset here for fabrics like * tcm_loop who may be using a contig buffer from the SCSI midlayer for @@ -2198,13 +2197,17 @@ void *transport_kmap_data_sg(struct se_cmd *cmd) */ if (!cmd->t_data_nents) return NULL; - else if (cmd->t_data_nents == 1) + + BUG_ON(!sg); + if (cmd->t_data_nents == 1) return kmap(sg_page(sg)) + sg->offset; /* >1 page. use vmap */ pages = kmalloc(sizeof(*pages) * cmd->t_data_nents, GFP_KERNEL); - if (!pages) + if (!pages) { + cmd->scsi_sense_reason = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; return NULL; + } /* convert sg[] to pages[] */ for_each_sg(cmd->t_data_sg, sg, cmd->t_data_nents, i) { @@ -2213,8 +2216,10 @@ void *transport_kmap_data_sg(struct se_cmd *cmd) cmd->t_data_vmap = vmap(pages, cmd->t_data_nents, VM_MAP, PAGE_KERNEL); kfree(pages); - if (!cmd->t_data_vmap) + if (!cmd->t_data_vmap) { + cmd->scsi_sense_reason = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; return NULL; + } return cmd->t_data_vmap + cmd->t_data_sg[0].offset; } -- cgit v0.10.2 From 32a8811ff164f882712c17946e58e52444f464a7 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 7 Sep 2012 17:30:36 +0200 Subject: target: support zero allocation length in REQUEST SENSE Similar to INQUIRY and MODE SENSE, construct the sense data in a buffer and later copy it to the scatterlist. Do not do anything, but still clear a pending unit attention condition, if the allocation length is zero. However, SPC tells us that "If a REQUEST SENSE command is terminated with CHECK CONDITION status [and] the REQUEST SENSE command was received on an I_T nexus with a pending unit attention condition (i.e., before the device server reports CHECK CONDITION status), then the device server shall not clear the pending unit attention condition." Do the transport_kmap_data_sg early to detect this case. It also tells us "Device servers shall not adjust the additional sense length to reflect truncation if the allocation length is less than the sense data available", so do not do that! Note that the err variable is write-only. Signed-off-by: Paolo Bonzini Signed-off-by: Nicholas Bellinger diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c index 4c861de..388a922 100644 --- a/drivers/target/target_core_spc.c +++ b/drivers/target/target_core_spc.c @@ -877,9 +877,11 @@ static int spc_emulate_modesense(struct se_cmd *cmd) static int spc_emulate_request_sense(struct se_cmd *cmd) { unsigned char *cdb = cmd->t_task_cdb; - unsigned char *buf; + unsigned char *rbuf; u8 ua_asc = 0, ua_ascq = 0; - int err = 0; + unsigned char buf[SE_SENSE_BUF]; + + memset(buf, 0, SE_SENSE_BUF); if (cdb[1] & 0x01) { pr_err("REQUEST_SENSE description emulation not" @@ -888,20 +890,21 @@ static int spc_emulate_request_sense(struct se_cmd *cmd) return -ENOSYS; } - buf = transport_kmap_data_sg(cmd); - - if (!core_scsi3_ua_clear_for_request_sense(cmd, &ua_asc, &ua_ascq)) { + rbuf = transport_kmap_data_sg(cmd); + if (cmd->scsi_sense_reason != 0) { + /* + * Out of memory. We will fail with CHECK CONDITION, so + * we must not clear the unit attention condition. + */ + target_complete_cmd(cmd, CHECK_CONDITION); + return 0; + } else if (!core_scsi3_ua_clear_for_request_sense(cmd, &ua_asc, &ua_ascq)) { /* * CURRENT ERROR, UNIT ATTENTION */ buf[0] = 0x70; buf[SPC_SENSE_KEY_OFFSET] = UNIT_ATTENTION; - if (cmd->data_length < 18) { - buf[7] = 0x00; - err = -EINVAL; - goto end; - } /* * The Additional Sense Code (ASC) from the UNIT ATTENTION */ @@ -915,11 +918,6 @@ static int spc_emulate_request_sense(struct se_cmd *cmd) buf[0] = 0x70; buf[SPC_SENSE_KEY_OFFSET] = NO_SENSE; - if (cmd->data_length < 18) { - buf[7] = 0x00; - err = -EINVAL; - goto end; - } /* * NO ADDITIONAL SENSE INFORMATION */ @@ -927,8 +925,11 @@ static int spc_emulate_request_sense(struct se_cmd *cmd) buf[7] = 0x0A; } -end: - transport_kunmap_data_sg(cmd); + if (rbuf) { + memcpy(rbuf, buf, min_t(u32, sizeof(buf), cmd->data_length)); + transport_kunmap_data_sg(cmd); + } + target_complete_cmd(cmd, GOOD); return 0; } diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h index 015cea0..5be8937 100644 --- a/include/target/target_core_base.h +++ b/include/target/target_core_base.h @@ -121,6 +121,7 @@ #define SE_INQUIRY_BUF 512 #define SE_MODE_PAGE_BUF 512 +#define SE_SENSE_BUF 96 /* struct se_hba->hba_flags */ enum hba_flags_table { -- cgit v0.10.2 From 6abbdf38363f064c8a50150c9b709682764483b3 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 7 Sep 2012 17:30:37 +0200 Subject: target: go through normal processing for zero-length REQUEST_SENSE Now that spc_emulate_request_sense has been taught to process zero-length REQUEST SENSE correctly, drop the special handling of unit attention conditions from transport_generic_new_cmd. However, for now REQUEST SENSE will be the only command that goes through emulation for zero lengths. (nab: Fix up zero-length check in transport_generic_new_cmd) Signed-off-by: Paolo Bonzini Signed-off-by: Nicholas Bellinger diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 4403477..269f544 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -2306,20 +2306,13 @@ int transport_generic_new_cmd(struct se_cmd *cmd) * away. */ if (!cmd->data_length && - (cmd->se_dev->transport->transport_type != TRANSPORT_PLUGIN_PHBA_PDEV || - cmd->t_task_cdb[0] == REPORT_LUNS) { + cmd->t_task_cdb[0] != REQUEST_SENSE && + cmd->se_dev->transport->transport_type != TRANSPORT_PLUGIN_PHBA_PDEV) { spin_lock_irq(&cmd->t_state_lock); cmd->t_state = TRANSPORT_COMPLETE; cmd->transport_state |= CMD_T_ACTIVE; spin_unlock_irq(&cmd->t_state_lock); - if (cmd->t_task_cdb[0] == REQUEST_SENSE) { - u8 ua_asc = 0, ua_ascq = 0; - - core_scsi3_ua_clear_for_request_sense(cmd, - &ua_asc, &ua_ascq); - } - INIT_WORK(&cmd->work, target_complete_ok_work); queue_work(target_completion_wq, &cmd->work); return 0; -- cgit v0.10.2