| Message ID | 1494523276.14920.2.camel@haakon3.risingtidesystems.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2017-05-11 at 10:21 -0700, Nicholas A. Bellinger wrote: > This reverts commit 0e2eb7d12eaa8e391bf5615d4271bb87a649caaa > > Author: Bart Van Assche <bart.vanassche@sandisk.com> > Date: Thu Mar 30 10:12:39 2017 -0700 > > target: Fix VERIFY and WRITE VERIFY command parsing > > This patch broke existing behaviour for WRITE_VERIFY because > it dropped the original SCF_SCSI_DATA_CDB assignment for > bytchk = 0 so target_cmd_size_check() no longer rejected > this case, allowing an overflow case to trigger an OOPs > in iscsi-target. > > Since the short term and long term fixes are still being > discussed, revert it for now since it's late in the merge > window and try again in v4.13-rc1. Hello Nic, In transport_generic_new_cmd(), called from iscsit_process_scsi_cmd() to submit an iSCSI command to the LIO core, one can see that target_alloc_sgls() is called whether or not SCF_SCSI_DATA_CDB has been set. So I don't think that what you wrote in the patch description is correct. If you have a look at my v2 patch series you will see that the buffer overflow I reported can be triggered for any SCSI command and not just for VERIFY / WRITE AND VERIFY. All that's needed to trigger a buffer overflow with the iSCSI target driver is to send about 4 * PAGE_SIZE bytes more immediate data than the Data-Out buffer size specified through the CDB. Bart.-- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2017-05-11 at 18:39 +0000, Bart Van Assche wrote: > On Thu, 2017-05-11 at 10:21 -0700, Nicholas A. Bellinger wrote: > > This reverts commit 0e2eb7d12eaa8e391bf5615d4271bb87a649caaa > > > > Author: Bart Van Assche <bart.vanassche@sandisk.com> > > Date: Thu Mar 30 10:12:39 2017 -0700 > > > > target: Fix VERIFY and WRITE VERIFY command parsing > > > > This patch broke existing behaviour for WRITE_VERIFY because > > it dropped the original SCF_SCSI_DATA_CDB assignment for > > bytchk = 0 so target_cmd_size_check() no longer rejected > > this case, allowing an overflow case to trigger an OOPs > > in iscsi-target. > > > > Since the short term and long term fixes are still being > > discussed, revert it for now since it's late in the merge > > window and try again in v4.13-rc1. > > Hello Nic, > > In transport_generic_new_cmd(), called from iscsit_process_scsi_cmd() to > submit an iSCSI command to the LIO core, one can see that target_alloc_sgls() > is called whether or not SCF_SCSI_DATA_CDB has been set. Wrong. iscsit_process_scsi_cmd() returns '1' to signal dump_payload to iscsit_get_immediate_data() when any CDB with SCF_SCSI_DATA_CDB is set. > So I don't think > that what you wrote in the patch description is correct. If you have a look > at my v2 patch series you will see that the buffer overflow I reported can > be triggered for any SCSI command and not just for VERIFY / WRITE AND VERIFY. > All that's needed to trigger a buffer overflow with the iSCSI target driver > is to send about 4 * PAGE_SIZE bytes more immediate data than the Data-Out > buffer size specified through the CDB. Considering a full page is still allocated for non SCF_SCSI_DATA_CDB, and iscsit_map_iovec() still checks for SGL array overflow, I don't think you'll be able to trigger anything. If you can, you should post the test case to prove it. :) Regardless, I'm going to push the change post -rc1 to address the regression that Roland's v4.3.y change introduced wrt to control WRITE CDB overflow: http://marc.info/?l=linux-scsi&m=149452576930990&w=2 Like I said earlier, you are more than welcome to add full WRITE CDB overflow support, but since it's a new feature it's not v4.12 material. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c index a0ad618..4316f7b 100644 --- a/drivers/target/target_core_sbc.c +++ b/drivers/target/target_core_sbc.c @@ -831,60 +831,6 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool succes return 0; } -/** - * sbc_parse_verify - parse VERIFY, VERIFY_16 and WRITE VERIFY commands - * @cmd: (in) structure that describes the SCSI command to be parsed. - * @sectors: (out) Number of logical blocks on the storage medium that will be - * affected by the SCSI command. - * @bufflen: (out) Expected length of the SCSI Data-Out buffer. - */ -static sense_reason_t sbc_parse_verify(struct se_cmd *cmd, int *sectors, - u32 *bufflen) -{ - struct se_device *dev = cmd->se_dev; - u8 *cdb = cmd->t_task_cdb; - u8 bytchk = (cdb[1] >> 1) & 3; - sense_reason_t ret; - - switch (cdb[0]) { - case VERIFY: - case WRITE_VERIFY: - *sectors = transport_get_sectors_10(cdb); - cmd->t_task_lba = transport_lba_32(cdb); - break; - case VERIFY_16: - case WRITE_VERIFY_16: - *sectors = transport_get_sectors_16(cdb); - cmd->t_task_lba = transport_lba_64(cdb); - break; - default: - WARN_ON_ONCE(true); - return TCM_UNSUPPORTED_SCSI_OPCODE; - } - - if (sbc_check_dpofua(dev, cmd, cdb)) - return TCM_INVALID_CDB_FIELD; - - ret = sbc_check_prot(dev, cmd, cdb, *sectors, true); - if (ret) - return ret; - - switch (bytchk) { - case 0: - *bufflen = 0; - break; - case 1: - *bufflen = sbc_get_size(cmd, *sectors); - cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB; - break; - default: - pr_err("Unsupported BYTCHK value %d for SCSI opcode %#x\n", - bytchk, cdb[0]); - return TCM_INVALID_CDB_FIELD; - } - return TCM_NO_SENSE; -} - sense_reason_t sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops) { @@ -952,6 +898,7 @@ static sense_reason_t sbc_parse_verify(struct se_cmd *cmd, int *sectors, cmd->execute_cmd = sbc_execute_rw; break; case WRITE_10: + case WRITE_VERIFY: sectors = transport_get_sectors_10(cdb); cmd->t_task_lba = transport_lba_32(cdb); @@ -965,13 +912,6 @@ static sense_reason_t sbc_parse_verify(struct se_cmd *cmd, int *sectors, cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB; cmd->execute_cmd = sbc_execute_rw; break; - case WRITE_VERIFY: - case WRITE_VERIFY_16: - ret = sbc_parse_verify(cmd, §ors, &size); - if (ret) - return ret; - cmd->execute_cmd = sbc_execute_rw; - goto check_lba; case WRITE_12: sectors = transport_get_sectors_12(cdb); cmd->t_task_lba = transport_lba_32(cdb); @@ -987,6 +927,7 @@ static sense_reason_t sbc_parse_verify(struct se_cmd *cmd, int *sectors, cmd->execute_cmd = sbc_execute_rw; break; case WRITE_16: + case WRITE_VERIFY_16: sectors = transport_get_sectors_16(cdb); cmd->t_task_lba = transport_lba_64(cdb); @@ -1169,9 +1110,14 @@ static sense_reason_t sbc_parse_verify(struct se_cmd *cmd, int *sectors, break; case VERIFY: case VERIFY_16: - ret = sbc_parse_verify(cmd, §ors, &size); - if (ret) - return ret; + size = 0; + if (cdb[0] == VERIFY) { + sectors = transport_get_sectors_10(cdb); + cmd->t_task_lba = transport_lba_32(cdb); + } else { + sectors = transport_get_sectors_16(cdb); + cmd->t_task_lba = transport_lba_64(cdb); + } cmd->execute_cmd = sbc_emulate_noop; goto check_lba; case REZERO_UNIT: