From: Nicholas Bellinger <[email protected]>
When iscsi WRITE underflow occurs there are two different scenarios
that can happen.
Normally in practice, when an EDTL vs. SCSI CDB TRANSFER LENGTH
underflow is detected, the iscsi immediate data payload is the
smaller SCSI CDB TRANSFER LENGTH.
That is, when a host fabric LLD is using a fixed size EDTL for
a specific control CDB, the SCSI CDB TRANSFER LENGTH and actual
SCSI payload ends up being smaller than EDTL. In iscsi, this
means the received iscsi immediate data payload matches the
smaller SCSI CDB TRANSFER LENGTH, because there is no more
SCSI payload to accept beyond SCSI CDB TRANSFER LENGTH.
However, it's possible for a malicous host to send a WRITE
underflow where EDTL is larger than SCSI CDB TRANSFER LENGTH,
but incoming iscsi immediate data actually matches EDTL.
In the wild, we've never had a iscsi host environment actually
try to do this.
For this special case, it's wrong to truncate part of the
control CDB payload and continue to process the command during
underflow when immediate data payload received was larger than
SCSI CDB TRANSFER LENGTH, so go ahead and reject and drop the
bogus payload as a defensive action.
Note this potential bug was originally relaxed by the following
for allowing WRITE underflow in MSFT FCP host environments:
commit c72c5250224d475614a00c1d7e54a67f77cd3410
Author: Roland Dreier <[email protected]>
Date: Wed Jul 22 15:08:18 2015 -0700
target: allow underflow/overflow for PR OUT etc. commands
Cc: Roland Dreier <[email protected]>
Cc: Mike Christie <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Martin K. Petersen <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
---
drivers/target/iscsi/iscsi_target.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index c025451..3fdca2c 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1279,6 +1279,18 @@ int iscsit_process_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
*/
if (dump_payload)
goto after_immediate_data;
+ /*
+ * Check for underflow case where both EDTL and immediate data payload
+ * exceeds what is presented by CDB's TRANSFER LENGTH, and what has
+ * already been set in target_cmd_size_check() as se_cmd->data_length.
+ *
+ * For this special case, fail the command and dump the immediate data
+ * payload.
+ */
+ if (cmd->first_burst_len > cmd->se_cmd.data_length) {
+ cmd->sense_reason = TCM_INVALID_CDB_FIELD;
+ goto after_immediate_data;
+ }
immed_ret = iscsit_handle_immediate_data(cmd, hdr,
cmd->first_burst_len);
--
1.9.1
On Thu, 2017-06-08 at 04:21 +0000, Nicholas A. Bellinger wrote:
> + /*
> + * Check for underflow case where both EDTL and immediate data payload
> + * exceeds what is presented by CDB's TRANSFER LENGTH, and what has
> + * already been set in target_cmd_size_check() as se_cmd->data_length.
> + *
> + * For this special case, fail the command and dump the immediate data
> + * payload.
> + */
> + if (cmd->first_burst_len > cmd->se_cmd.data_length) {
> + cmd->sense_reason = TCM_INVALID_CDB_FIELD;
> + goto after_immediate_data;
> + }
A quote from the iSCSI RFC (https://tools.ietf.org/html/rfc5048):
If SPDTL < EDTL for a task, iSCSI Underflow MUST be signaled in the
???SCSI Response PDU as specified in [RFC3720].??The Residual Count MUST
???be set to the numerical value of (EDTL - SPDTL).
Sorry but I don't think that sending TCM_INVALID_CDB_FIELD back to the
initiator is compliant with the iSCSI RFC. Please note that a fix that is
compliant with the iSCSI RFC is present in the following patch series: [PATCH
00/33] SCSI target driver patches for kernel v4.13, 23 May 2017
(https://www.spinics.net/lists/target-devel/msg15370.html).
Bart.
On Thu, 2017-06-08 at 15:37 +0000, Bart Van Assche wrote:
> On Thu, 2017-06-08 at 04:21 +0000, Nicholas A. Bellinger wrote:
> > + /*
> > + * Check for underflow case where both EDTL and immediate data payload
> > + * exceeds what is presented by CDB's TRANSFER LENGTH, and what has
> > + * already been set in target_cmd_size_check() as se_cmd->data_length.
> > + *
> > + * For this special case, fail the command and dump the immediate data
> > + * payload.
> > + */
> > + if (cmd->first_burst_len > cmd->se_cmd.data_length) {
> > + cmd->sense_reason = TCM_INVALID_CDB_FIELD;
> > + goto after_immediate_data;
> > + }
>
> A quote from the iSCSI RFC (https://tools.ietf.org/html/rfc5048):
>
> If SPDTL < EDTL for a task, iSCSI Underflow MUST be signaled in the
> SCSI Response PDU as specified in [RFC3720]. The Residual Count MUST
> be set to the numerical value of (EDTL - SPDTL).
>
> Sorry but I don't think that sending TCM_INVALID_CDB_FIELD back to the
> initiator is compliant with the iSCSI RFC.
Alas, the nuance of what this patch actually does was missed when you
cut the context.
First, a bit of history. LIO has rejected underflow for all WRITEs for
the first ~12.5 years of RFC-3720, and in the context of iscsi-target
mode there has never been a single host environment that ever once
cared.
Since Roland's patch to allow underflow for control CDBs in v4.3+ opened
this discussion for control CDBs with a WRITE payload in order to make
MSFT/FCP cert for PERSISTENT_RESERVE_OUT happy, the question has become
what control CDB WRITE underflow cases should we allow..?
The point with this patch is when a host is sending a underflow with a
iscsi immediate data payload that exceeds SCSI transfer length, it's a
bogus request with a garbage payload. It's a garbage payload because
the SCSI CDB itself obviously doesn't want anything to do it.
I'm very dubious of any host environment who's trying to do this for any
CDB, and expects achieve expected results.
Of course, since v4.3+ normal overflow where SCSI transfer length
matches the iscsi immediate data payload just works with or without this
patch.
So to that extent, I'm going to push this patch as a defensive fix for
v4.3+, to let those imaginary iscsi host environments know they being
very, very naughty.
> Please note that a fix that is
> compliant with the iSCSI RFC is present in the following patch series: [PATCH
> 00/33] SCSI target driver patches for kernel v4.13, 23 May 2017
> (https://www.spinics.net/lists/target-devel/msg15370.html).
So I might still consider this as a v4.13-rc item for control CDB
underflow, but no way as stable material.
Also, there is certainly no way I'm going to allow a patch to randomly
enable underflow/overflow for all WRITE non control CDBs tree-wide
across all fabric drivers, because 1) no host environments actually care
about this, and 2) it's still dangerous to do for all fabrics without
some serious auditing.
Hi Bart,
On Thu, 2017-06-08 at 23:55 -0700, Nicholas A. Bellinger wrote:
> On Thu, 2017-06-08 at 15:37 +0000, Bart Van Assche wrote:
> > On Thu, 2017-06-08 at 04:21 +0000, Nicholas A. Bellinger wrote:
> > > + /*
> > > + * Check for underflow case where both EDTL and immediate data payload
> > > + * exceeds what is presented by CDB's TRANSFER LENGTH, and what has
> > > + * already been set in target_cmd_size_check() as se_cmd->data_length.
> > > + *
> > > + * For this special case, fail the command and dump the immediate data
> > > + * payload.
> > > + */
> > > + if (cmd->first_burst_len > cmd->se_cmd.data_length) {
> > > + cmd->sense_reason = TCM_INVALID_CDB_FIELD;
> > > + goto after_immediate_data;
> > > + }
> >
> > A quote from the iSCSI RFC (https://tools.ietf.org/html/rfc5048):
> >
> > If SPDTL < EDTL for a task, iSCSI Underflow MUST be signaled in the
> > SCSI Response PDU as specified in [RFC3720]. The Residual Count MUST
> > be set to the numerical value of (EDTL - SPDTL).
> >
> > Sorry but I don't think that sending TCM_INVALID_CDB_FIELD back to the
> > initiator is compliant with the iSCSI RFC.
>
> Alas, the nuance of what this patch actually does was missed when you
> cut the context.
>
> First, a bit of history. LIO has rejected underflow for all WRITEs for
> the first ~12.5 years of RFC-3720, and in the context of iscsi-target
> mode there has never been a single host environment that ever once
> cared.
>
> Since Roland's patch to allow underflow for control CDBs in v4.3+ opened
> this discussion for control CDBs with a WRITE payload in order to make
> MSFT/FCP cert for PERSISTENT_RESERVE_OUT happy, the question has become
> what control CDB WRITE underflow cases should we allow..?
>
> The point with this patch is when a host is sending a underflow with a
> iscsi immediate data payload that exceeds SCSI transfer length, it's a
> bogus request with a garbage payload. It's a garbage payload because
> the SCSI CDB itself obviously doesn't want anything to do it.
>
> I'm very dubious of any host environment who's trying to do this for any
> CDB, and expects achieve expected results.
>
> Of course, since v4.3+ normal overflow where SCSI transfer length
> matches the iscsi immediate data payload just works with or without this
> patch.
>
> So to that extent, I'm going to push this patch as a defensive fix for
> v4.3+, to let those imaginary iscsi host environments know they being
> very, very naughty.
>
> > Please note that a fix that is
> > compliant with the iSCSI RFC is present in the following patch series: [PATCH
> > 00/33] SCSI target driver patches for kernel v4.13, 23 May 2017
> > (https://www.spinics.net/lists/target-devel/msg15370.html).
>
> So I might still consider this as a v4.13-rc item for control CDB
> underflow, but no way as stable material.
>
> Also, there is certainly no way I'm going to allow a patch to randomly
> enable underflow/overflow for all WRITE non control CDBs tree-wide
> across all fabric drivers, because 1) no host environments actually care
> about this, and 2) it's still dangerous to do for all fabrics without
> some serious auditing.
After further consideration, I've decided against allowing iscsi-target
underflow with a immediate data payload larger than SCSI transfer
length.
Any host environment that attempts to send an underflow with a immediate
data payload larger than SCSI transfer length, expects the target to
automatically truncate immediate data, and still return GOOD status is
completely bogus. Any host that attempts this is buggy, and needs to be
fixed.
This is because for the last ~12 years of RFC-3720:
- There has never been a host environment in the wild that exhibits
this behavior.
- There has never been a conformance suite which expects this
behavior.
So rejecting this case as already done in commit abb85a9b51 is the
correct approach for >= v4.3.y.
Of course, the typical underflow scenario which Roland's v4.3.y commit
enabled, underflow where immediate data matches the SCSI transfer length
is supported for control CDBs.
That said, thanks for high-lighting this particular corner case, so it
could be fixed in >= v4.3.y.
On Tue, 2017-07-11 at 00:22 -0700, Nicholas A. Bellinger wrote:
> So rejecting this case as already done in commit abb85a9b51 is the
> correct approach for >= v4.3.y.
Hello Nic,
I hope that you agree that the current target_cmd_size_check() implementation
is complicated and ugly. Patch 30/33 of the patch series I referred to in my
e-mail removes a significant number of lines of code from that function. So
my patch series not only makes target_cmd_size_check() easier to maintain and
to verify but it makes that function also faster. Hence please reconsider the
approach from my patch series. For patch 30/33, see also
https://www.spinics.net/lists/target-devel/msg15384.html.
Bart.
On Tue, 2017-07-11 at 16:17 +0000, Bart Van Assche wrote:
> On Tue, 2017-07-11 at 00:22 -0700, Nicholas A. Bellinger wrote:
> > So rejecting this case as already done in commit abb85a9b51 is the
> > correct approach for >= v4.3.y.
>
> Hello Nic,
>
> I hope that you agree that the current target_cmd_size_check() implementation
> is complicated and ugly. Patch 30/33 of the patch series I referred to in my
> e-mail removes a significant number of lines of code from that function. So
> my patch series not only makes target_cmd_size_check() easier to maintain and
> to verify but it makes that function also faster. Hence please reconsider the
> approach from my patch series. For patch 30/33, see also
> https://www.spinics.net/lists/target-devel/msg15384.html.
For reference, here is the patch your advocating:
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index c28e3b58150b..6cd49fe578a7 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1164,23 +1164,6 @@ target_cmd_size_check(struct se_cmd *cmd, unsigned int size)
" %u does not match SCSI CDB Length: %u for SAM Opcode:"
" 0x%02x\n", cmd->se_tfo->get_fabric_name(),
cmd->data_length, size, cmd->t_task_cdb[0]);
-
- if (cmd->data_direction == DMA_TO_DEVICE &&
- cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) {
- pr_err("Rejecting underflow/overflow WRITE data\n");
- return TCM_INVALID_CDB_FIELD;
- }
- /*
- * Reject READ_* or WRITE_* with overflow/underflow for
- * type SCF_SCSI_DATA_CDB.
- */
- if (dev->dev_attrib.block_size != 512) {
- pr_err("Failing OVERFLOW/UNDERFLOW for LBA op"
- " CDB on non 512-byte sector setup subsystem"
- " plugin: %s\n", dev->transport->name);
- /* Returns CHECK_CONDITION + INVALID_CDB_FIELD */
- return TCM_INVALID_CDB_FIELD;
- }
/*
* For the overflow case keep the existing fabric provided
* ->data_length. Otherwise for the underflow case, reset
The original code is not 'complicated' or 'ugly', and the proposed
change doesn't make anything 'faster' nor removes a 'significant' number
of LoC.
So no, I don't buy any of that line of reasoning. ;-)
It comes down to two items. First, if SCF_SCSI_DATA_CDBs with
underflow/overflow will be allowed, and second the block_size != 512
check.
For the former, I've still never seen a host environment in the wild
over the last 15 years that generates underflow/overflow for DATA CDBs
with an LBA. So I'm reluctant to randomly allow this for all cases and
fabrics, considering no host environment actually requires it.
That said, I do understand libiscsi generates this for WRITE_VERIFY, but
I'm still undecided if that's a good enough a reason to enable it for
all cases in upstream.
For the latter item, it's fine to drop the legacy block_size != 512
check, and I'll take a patch for that separate from the other bit.
On Thu, 2017-07-13 at 12:27 -0700, Nicholas A. Bellinger wrote:
> For the former, I've still never seen a host environment in the wild
> over the last 15 years that generates underflow/overflow for DATA CDBs
> with an LBA. So I'm reluctant to randomly allow this for all cases and
> fabrics, considering no host environment actually requires it.
>
> That said, I do understand libiscsi generates this for WRITE_VERIFY, but
> I'm still undecided if that's a good enough a reason to enable it for
> all cases in upstream.
>
> For the latter item, it's fine to drop the legacy block_size != 512
> check, and I'll take a patch for that separate from the other bit.
Hello Nic,
Thanks for considering to drop the block_size != 512 check.
There is one side effect of the current code for handling WRITE underflows
that has not yet been mentioned in this e-mail thread. The current
implementation of the iSCSI target driver does not discard the entire
immediate data buffer if the size of that buffer is larger than the data
buffer size derived from the SCSI CDB (EDTL). Because of this the iSCSI
target driver will attempt to parse some of the immediate data as an iSCSI
PDU. This can cause very weird failures and error messages. I think we
should address this and also that we should make sure that any iSCSI PDUs
that follow a too large immediate data buffer are parsed correctly.
Bart.