diff mbox

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

Message ID 1494197759.30469.39.camel@haakon3.risingtidesystems.com (mailing list archive)
State New, archived
Headers show

Commit Message

Nicholas A. Bellinger May 7, 2017, 10:55 p.m. UTC 
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.



--
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

Comments

Bart Van Assche May 8, 2017, 6:10 p.m. UTC | #1 
On Sun, 2017-05-07 at 15:55 -0700, Nicholas A. Bellinger wrote:
> 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;

As I have already explained in another e-mail: the above change is completely wrong.

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
Nicholas A. Bellinger May 10, 2017, 4:32 a.m. UTC | #2 
On Mon, 2017-05-08 at 18:10 +0000, Bart Van Assche wrote:
> On Sun, 2017-05-07 at 15:55 -0700, Nicholas A. Bellinger wrote:
> > 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;
> 
> As I have already explained in another e-mail: the above change is completely wrong.

Nope.

If your original patch had been run through libiscsi before posting it,
you'd have already caught the regression.

So run it though libiscsi and see for yourself that it restores existing
behavior.

In any event, this is the patch I'll be merging for -rc1 to restore
existing behavior to avoid the OOPsen your change has introduced.

--
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
Bart Van Assche May 10, 2017, 4:12 p.m. UTC | #3 
On Tue, 2017-05-09 at 21:32 -0700, Nicholas A. Bellinger wrote:
> On Mon, 2017-05-08 at 18:10 +0000, Bart Van Assche wrote:
> > [ ... ]
> > As I have already explained in another e-mail: the above change is completely wrong.
> 
> In any event, this is the patch I'll be merging for -rc1 to restore
> existing behavior to avoid the OOPsen your change has introduced.

Hello Nic,

In case it wouldn't be clear, my reply counts as a NAK. Please add the
following to that patch before you send it to Linus:

NAK-ed-by: Bart Van Assche <Bart.VanAssche@sandisk.com>

Thanks,

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
Nicholas A. Bellinger May 11, 2017, 5:22 a.m. UTC | #4 
On Wed, 2017-05-10 at 16:12 +0000, Bart Van Assche wrote:
> On Tue, 2017-05-09 at 21:32 -0700, Nicholas A. Bellinger wrote:
> > On Mon, 2017-05-08 at 18:10 +0000, Bart Van Assche wrote:
> > > [ ... ]
> > > As I have already explained in another e-mail: the above change is completely wrong.
> > 
> > In any event, this is the patch I'll be merging for -rc1 to restore
> > existing behavior to avoid the OOPsen your change has introduced.
> 
> Hello Nic,
> 
> In case it wouldn't be clear, my reply counts as a NAK. Please add the
> following to that patch before you send it to Linus:
> 
> NAK-ed-by: Bart Van Assche <Bart.VanAssche@sandisk.com>
> 

Listen, your patch in target-pending/for-next broke existing behavior
for WRITE_VERIFY because you never bothered to test the original patch
with overflow, so I'm going to include this bug-fix in my PULL request
so it doesn't trigger an OOPs.

Otherwise I'll just revert your original patch and be done with it for
-rc1.

Which do you prefer..?

--
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 mbox

Patch

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;