lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAPj3X_W5kOEOapG3F8NETBRzBmrQ1Lfudy7QGmCLXPT3UwUrkw@mail.gmail.com>
Date:   Wed, 13 Dec 2023 17:24:54 -0800
From:   Lee Duncan <lduncan@...e.com>
To:     Chris Leech <cleech@...hat.com>
Cc:     target-devel@...r.kernel.org, linux-scsi@...r.kernel.org,
        linux-kernel@...r.kernel.org, dbond@...e.com, hare@...e.de,
        michael.christie@...cle.com
Subject: Re: [PATCH 1/2] scsi: target: iscsi: handle SCSI immediate commands

Apologies on my first reply having HTML. I'm learning a new MUA.

On Wed, Dec 13, 2023 at 12:06 PM Chris Leech <cleech@...hat.com> wrote:
>
> On Thu, Dec 07, 2023 at 09:42:34AM -0800, lduncan@...e.com wrote:
> > From: Lee Duncan <lduncan@...e.com>
> >
> > Some iSCSI initiators send SCSI PDUs with the "immediate" bit
> > set, and this is allowed according to RFC 3720. Commands with
> > the "Immediate" bit set are called "immediate commands". From
> > section 3.2.2.1. "Command Numbering and Acknowledging":
> >
> >    The target MUST NOT transmit a MaxCmdSN that is less than
> >    ExpCmdSN-1.  For non-immediate commands, the CmdSN field can take any
> >    value from ExpCmdSN to MaxCmdSN inclusive.  The target MUST silently
> >    ignore any non-immediate command outside of this range or non-
> >    immediate duplicates within the range.  The CmdSN carried by
> >    immediate commands may lie outside the ExpCmdSN to MaxCmdSN range.
> >    For example, if the initiator has previously sent a non-immediate
> >    command carrying the CmdSN equal to MaxCmdSN, the target window is
> >    closed.  For group task management commands issued as immediate
> >    commands, CmdSN indicates the scope of the group action (e.g., on
> >    ABORT TASK SET indicates which commands are aborted).
> >
> > This fixed an issue with fastlinq qedi Converged Network Adapter
> > initiator firmware, trying to use an LIO target for booting. These
> > changes made booting possible, with or without ImmediateData enabled.
> >
> > Signed-off-by: Lee Duncan <lduncan@...e.com>
> > Reviewed-by: David Bond <dbond@...e.com>
> > ---
> >  drivers/target/iscsi/iscsi_target.c      | 12 +++---------
> >  drivers/target/iscsi/iscsi_target_util.c | 10 ++++++++--
> >  2 files changed, 11 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> > index 1d25e64b068a..f246e5015868 100644
> > --- a/drivers/target/iscsi/iscsi_target.c
> > +++ b/drivers/target/iscsi/iscsi_target.c
> > @@ -1060,13 +1060,6 @@ int iscsit_setup_scsi_cmd(struct iscsit_conn *conn, struct iscsit_cmd *cmd,
> >                                            ISCSI_REASON_BOOKMARK_INVALID, buf);
> >       }
> >
> > -     if (hdr->opcode & ISCSI_OP_IMMEDIATE) {
> > -             pr_err("Illegally set Immediate Bit in iSCSI Initiator"
> > -                             " Scsi Command PDU.\n");
> > -             return iscsit_add_reject_cmd(cmd,
> > -                                          ISCSI_REASON_BOOKMARK_INVALID, buf);
> > -     }
> > -
> >       if (payload_length && !conn->sess->sess_ops->ImmediateData) {
> >               pr_err("ImmediateData=No but DataSegmentLength=%u,"
> >                       " protocol error.\n", payload_length);
>
> This seems right, as the flag is checked again later in the same
> function.
>
> > @@ -1255,14 +1248,15 @@ int iscsit_process_scsi_cmd(struct iscsit_conn *conn, struct iscsit_cmd *cmd,
> >       /*
> >        * Check the CmdSN against ExpCmdSN/MaxCmdSN here if
> >        * the Immediate Bit is not set, and no Immediate
> > -      * Data is attached.
> > +      * Data is attached. Also skip the check if this is
> > +      * an immediate command.
>
> This comment addition seems redundant, isn't that what the "Immediate
> Bit is not set" already means?

The spec is confusing with respect to this. The "Immediate Bit"
means an immediate command. These commands are done "now",
not queued, and they do not increment the expected sequence number.

Immediate data is different, and unfortunately named IMHO. It's when a
PDU supplies the data for the SCSI command in the current PDU instead
of the next PDU.

>
> >        *
> >        * A PDU/CmdSN carrying Immediate Data can only
> >        * be processed after the DataCRC has passed.
> >        * If the DataCRC fails, the CmdSN MUST NOT
> >        * be acknowledged. (See below)
> >        */
> > -     if (!cmd->immediate_data) {
> > +     if (!cmd->immediate_data && !cmd->immediate_cmd) {
> >               cmdsn_ret = iscsit_sequence_cmd(conn, cmd,
> >                                       (unsigned char *)hdr, hdr->cmdsn);
> >               if (cmdsn_ret == CMDSN_ERROR_CANNOT_RECOVER)
>
> Are you sure this needs to be checking both conditions here?  I'm
> struggling to understand why CmdSN checking would be bypassed for
> immediate data.  Is this a longstanding bug where the condition should
> have been on immediate_cmd (and only immediate_cmd) instead?

The immediate data check was there already, and there haven't been any
bugs I know of, so I assumed that part of the code was ok.

>
> Or is this because of the handling the immediate data with DataCRC case
> mentioned?  I do see iscsit_sequence_cmd also being called in
> iscsit_get_immediate_data.

I will check that but I suspect you are correct.

>
> - Chris Leech
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ