Re: [PATCH 18/19] target/iscsi: Avoid that CDB parser bugs trigger a kernel crash

["Nicholas A. Bellinger" <nab@xxxxxxxxxxxxxxx>]

On Thu, 2017-05-04 at 15:51 -0700, Bart Van Assche wrote:
> If the code for parsing a CDB sets se_cmd.data_length to a value
> that is lower than the size of the SCSI Data-Out buffer then a
> buffer overflow occurs in the iSCSI target driver while receiving
> the Data-Out buffer. Make the code for receiving that buffer more
> robust by checking the bounds of the allocated iovec. This patch
> fixes the following crash:
> 
> BUG: unable to handle kernel NULL pointer dereference at 00000000
> 00000014
> RIP: 0010:iscsit_map_iovec+0x120/0x190 [iscsi_target_mod]
> Call Trace:
>  iscsit_get_rx_pdu+0x8a2/0xe00 [iscsi_target_mod]
>  iscsi_target_rx_thread+0x6e/0xa0 [iscsi_target_mod]
>  kthread+0x109/0x140
> 

Again, this OOPs is a consequence of your earlier sbc_parse_verify()
patch not setting SCF_SCSI_DATA_CDB and allowing unsupported WRITE
overflow to be processed.

Here is what I'm applying to for-next to address these regressions.

First, is to always set SCF_SCSI_DATA_CDB when we expect to transfer
data regardless of bytchk, and don't incorrect set the 'bufflen = 0' for
bytchk = 0 because it will still be transferring data.

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index a0ad618..2cc8753 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -836,10 +836,8 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool succes
  * @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)
+static sense_reason_t sbc_parse_verify(struct se_cmd *cmd, unsigned int *sectors)
 {
        struct se_device *dev = cmd->se_dev;
        u8 *cdb = cmd->t_task_cdb;
@@ -871,10 +869,7 @@ static sense_reason_t sbc_parse_verify(struct se_cmd *cmd, int *sectors,
 
        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:
@@ -967,7 +962,7 @@ static sense_reason_t sbc_parse_verify(struct se_cmd *cmd, int *sectors,
                break;
        case WRITE_VERIFY:
        case WRITE_VERIFY_16:
-               ret = sbc_parse_verify(cmd, &sectors, &size);
+               ret = sbc_parse_verify(cmd, &sectors);
                if (ret)
                        return ret;
                cmd->execute_cmd = sbc_execute_rw;
@@ -1169,7 +1164,7 @@ static sense_reason_t sbc_parse_verify(struct se_cmd *cmd, int *sectors,
                break;
        case VERIFY:
        case VERIFY_16:
-               ret = sbc_parse_verify(cmd, &sectors, &size);
+               ret = sbc_parse_verify(cmd, &sectors);
                if (ret)
                        return ret;
                cmd->execute_cmd = sbc_emulate_noop;


--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html