* [PATCH 1/3] target: Fix sense data for out-of-bounds IO operations
@ 2013-02-08 23:18 Roland Dreier
2013-02-08 23:18 ` [PATCH 2/3] target: Fix error checking for UNMAP commands Roland Dreier
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Roland Dreier @ 2013-02-08 23:18 UTC (permalink / raw)
To: Nicholas A. Bellinger; +Cc: target-devel, linux-scsi, Roland Dreier
From: Roland Dreier <roland@purestorage.com>
We're supposed to return LOGICAL BLOCK ADDRESS OUT OF RANGE, not
INVALID FIELD IN CDB.
Signed-off-by: Roland Dreier <roland@purestorage.com>
---
drivers/target/target_core_sbc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index a664c66..170f1f7 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -486,7 +486,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
*/
if (cmd->t_task_lba || sectors) {
if (sbc_check_valid_sectors(cmd) < 0)
- return TCM_INVALID_CDB_FIELD;
+ return TCM_ADDRESS_OUT_OF_RANGE;
}
cmd->execute_cmd = ops->execute_sync_cache;
break;
--
1.8.1.2
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 2/3] target: Fix error checking for UNMAP commands 2013-02-08 23:18 [PATCH 1/3] target: Fix sense data for out-of-bounds IO operations Roland Dreier @ 2013-02-08 23:18 ` Roland Dreier 2013-02-09 9:23 ` Chris Boot 2013-02-13 20:27 ` Nicholas A. Bellinger 2013-02-08 23:18 ` [PATCH 3/3] target: Fix parameter list length checking in MODE SELECT Roland Dreier 2013-02-13 20:26 ` [PATCH 1/3] target: Fix sense data for out-of-bounds IO operations Nicholas A. Bellinger 2 siblings, 2 replies; 8+ messages in thread From: Roland Dreier @ 2013-02-08 23:18 UTC (permalink / raw) To: Nicholas A. Bellinger; +Cc: target-devel, linux-scsi, Roland Dreier From: Roland Dreier <roland@purestorage.com> SBC-3 (revision 35) says: The PARAMETER LIST LENGTH field specifies the length in bytes of the UNMAP parameter list that is available to be transferred from the Data-Out Buffer. If the parameter list length is greater than zero and less than 0008h (i.e., eight), then the device server shall terminate the command with CHECK CONDITION status with the sense key set to ILLEGAL REQUEST and the additional sense code set to PARAMETER LIST LENGTH ERROR. A PARAMETER LIST LENGTH set to zero specifies that no data shall be sent. so our sense code for too-short descriptors was wrong, and we were incorrectly failing commands that didn't transfer any descriptors. While we're at it, also handle the UNMAP check: If the ANCHOR bit is set to one, and the ANC_SUP bit in the Logical Block Provisioning VPD page (see 6.6.4) is set to zero, then the device server shall terminate the command with CHECK CONDITION status with the sense key set to ILLEGAL REQUEST and the additional sense code set to INVALID FIELD IN CDB. Signed-off-by: Roland Dreier <roland@purestorage.com> --- drivers/target/target_core_iblock.c | 11 ++++++++++- drivers/target/target_core_transport.c | 10 ++++++++++ include/target/target_core_base.h | 1 + 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c index b526d23..b72ca5b 100644 --- a/drivers/target/target_core_iblock.c +++ b/drivers/target/target_core_iblock.c @@ -390,10 +390,19 @@ iblock_execute_unmap(struct se_cmd *cmd) sense_reason_t ret = 0; int dl, bd_dl, err; + /* We never set ANC_SUP */ + if (cdb[1]) + return TCM_INVALID_CDB_FIELD; + + if (cmd->data_length == 0) { + target_complete_cmd(cmd, SAM_STAT_GOOD); + return 0; + } + if (cmd->data_length < 8) { pr_warn("UNMAP parameter list length %u too small\n", cmd->data_length); - return TCM_INVALID_PARAMETER_LIST; + return TCM_PARAMETER_LIST_LENGTH_ERROR; } buf = transport_kmap_data_sg(cmd); diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index bd587b7..82c4204 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -1514,6 +1514,7 @@ void transport_generic_request_failure(struct se_cmd *cmd, case TCM_UNSUPPORTED_SCSI_OPCODE: case TCM_INVALID_CDB_FIELD: case TCM_INVALID_PARAMETER_LIST: + case TCM_PARAMETER_LIST_LENGTH_ERROR: case TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE: case TCM_UNKNOWN_MODE_PAGE: case TCM_WRITE_PROTECTED: @@ -2674,6 +2675,15 @@ transport_send_check_condition_and_sense(struct se_cmd *cmd, /* INVALID FIELD IN PARAMETER LIST */ buffer[SPC_ASC_KEY_OFFSET] = 0x26; break; + case TCM_PARAMETER_LIST_LENGTH_ERROR: + /* CURRENT ERROR */ + buffer[0] = 0x70; + buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10; + /* ILLEGAL REQUEST */ + buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST; + /* INVALID FIELD IN PARAMETER LIST */ + buffer[SPC_ASC_KEY_OFFSET] = 0x1a; + break; case TCM_UNEXPECTED_UNSOLICITED_DATA: /* CURRENT ERROR */ buffer[0] = 0x70; diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h index 4fa0f10..37e3baa 100644 --- a/include/target/target_core_base.h +++ b/include/target/target_core_base.h @@ -193,6 +193,7 @@ enum tcm_sense_reason_table { TCM_RESERVATION_CONFLICT = R(0x10), TCM_ADDRESS_OUT_OF_RANGE = R(0x11), TCM_OUT_OF_RESOURCES = R(0x12), + TCM_PARAMETER_LIST_LENGTH_ERROR = R(0x13), #undef R }; -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] target: Fix error checking for UNMAP commands 2013-02-08 23:18 ` [PATCH 2/3] target: Fix error checking for UNMAP commands Roland Dreier @ 2013-02-09 9:23 ` Chris Boot 2013-02-09 19:27 ` Roland Dreier 2013-02-13 20:27 ` Nicholas A. Bellinger 1 sibling, 1 reply; 8+ messages in thread From: Chris Boot @ 2013-02-09 9:23 UTC (permalink / raw) To: Roland Dreier, Nicholas A. Bellinger Cc: target-devel, linux-scsi, Roland Dreier On 08/02/2013 23:18, Roland Dreier wrote: > diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c > index b526d23..b72ca5b 100644 > --- a/drivers/target/target_core_iblock.c > +++ b/drivers/target/target_core_iblock.c > @@ -2674,6 +2675,15 @@ transport_send_check_condition_and_sense(struct se_cmd *cmd, > /* INVALID FIELD IN PARAMETER LIST */ > buffer[SPC_ASC_KEY_OFFSET] = 0x26; > break; > + case TCM_PARAMETER_LIST_LENGTH_ERROR: > + /* CURRENT ERROR */ > + buffer[0] = 0x70; > + buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10; > + /* ILLEGAL REQUEST */ > + buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST; > + /* INVALID FIELD IN PARAMETER LIST */ > + buffer[SPC_ASC_KEY_OFFSET] = 0x1a; > + break; > case TCM_UNEXPECTED_UNSOLICITED_DATA: > /* CURRENT ERROR */ > buffer[0] = 0x70; Nitpick: I suspect a simple copy & paste error; "INVALID FIELD IN PARAMETER LIST" in your comment should probably read "PARAMETER LIST LENGTH ERROR" instead. HTH, Chris -- Chris Boot bootc@bootc.net ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] target: Fix error checking for UNMAP commands 2013-02-09 9:23 ` Chris Boot @ 2013-02-09 19:27 ` Roland Dreier 0 siblings, 0 replies; 8+ messages in thread From: Roland Dreier @ 2013-02-09 19:27 UTC (permalink / raw) To: Chris Boot; +Cc: Nicholas A. Bellinger, target-devel, linux-scsi On Sat, Feb 9, 2013 at 1:23 AM, Chris Boot <bootc@bootc.net> wrote: >> + case TCM_PARAMETER_LIST_LENGTH_ERROR: >> + /* CURRENT ERROR */ >> + buffer[0] = 0x70; >> + buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10; >> + /* ILLEGAL REQUEST */ >> + buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST; >> + /* INVALID FIELD IN PARAMETER LIST */ >> + buffer[SPC_ASC_KEY_OFFSET] = 0x1a; >> + break; >> case TCM_UNEXPECTED_UNSOLICITED_DATA: >> /* CURRENT ERROR */ >> buffer[0] = 0x70; > > > Nitpick: I suspect a simple copy & paste error; "INVALID FIELD IN PARAMETER > LIST" in your comment should probably read "PARAMETER LIST LENGTH ERROR" > instead. Yes, the comment is a copy and paste error. The ASC of 1Ah is correct for PARAMETER LIST LENGTH ERROR. Thanks & good catch. Nic, want me to resend? - R. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] target: Fix error checking for UNMAP commands 2013-02-08 23:18 ` [PATCH 2/3] target: Fix error checking for UNMAP commands Roland Dreier 2013-02-09 9:23 ` Chris Boot @ 2013-02-13 20:27 ` Nicholas A. Bellinger 1 sibling, 0 replies; 8+ messages in thread From: Nicholas A. Bellinger @ 2013-02-13 20:27 UTC (permalink / raw) To: Roland Dreier; +Cc: target-devel, linux-scsi, Roland Dreier On Fri, 2013-02-08 at 15:18 -0800, Roland Dreier wrote: > From: Roland Dreier <roland@purestorage.com> > > SBC-3 (revision 35) says: > > The PARAMETER LIST LENGTH field specifies the length in bytes of the > UNMAP parameter list that is available to be transferred from the > Data-Out Buffer. If the parameter list length is greater than zero > and less than 0008h (i.e., eight), then the device server shall > terminate the command with CHECK CONDITION status with the sense key > set to ILLEGAL REQUEST and the additional sense code set to > PARAMETER LIST LENGTH ERROR. A PARAMETER LIST LENGTH set to zero > specifies that no data shall be sent. > > so our sense code for too-short descriptors was wrong, and we were > incorrectly failing commands that didn't transfer any descriptors. > > While we're at it, also handle the UNMAP check: > > If the ANCHOR bit is set to one, and the ANC_SUP bit in the Logical > Block Provisioning VPD page (see 6.6.4) is set to zero, then the > device server shall terminate the command with CHECK CONDITION > status with the sense key set to ILLEGAL REQUEST and the additional > sense code set to INVALID FIELD IN CDB. > > Signed-off-by: Roland Dreier <roland@purestorage.com> > --- Applied with Chris Boot's cut+paste comment fix. Thanks, --nab > drivers/target/target_core_iblock.c | 11 ++++++++++- > drivers/target/target_core_transport.c | 10 ++++++++++ > include/target/target_core_base.h | 1 + > 3 files changed, 21 insertions(+), 1 deletion(-) > > diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c > index b526d23..b72ca5b 100644 > --- a/drivers/target/target_core_iblock.c > +++ b/drivers/target/target_core_iblock.c > @@ -390,10 +390,19 @@ iblock_execute_unmap(struct se_cmd *cmd) > sense_reason_t ret = 0; > int dl, bd_dl, err; > > + /* We never set ANC_SUP */ > + if (cdb[1]) > + return TCM_INVALID_CDB_FIELD; > + > + if (cmd->data_length == 0) { > + target_complete_cmd(cmd, SAM_STAT_GOOD); > + return 0; > + } > + > if (cmd->data_length < 8) { > pr_warn("UNMAP parameter list length %u too small\n", > cmd->data_length); > - return TCM_INVALID_PARAMETER_LIST; > + return TCM_PARAMETER_LIST_LENGTH_ERROR; > } > > buf = transport_kmap_data_sg(cmd); > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index bd587b7..82c4204 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -1514,6 +1514,7 @@ void transport_generic_request_failure(struct se_cmd *cmd, > case TCM_UNSUPPORTED_SCSI_OPCODE: > case TCM_INVALID_CDB_FIELD: > case TCM_INVALID_PARAMETER_LIST: > + case TCM_PARAMETER_LIST_LENGTH_ERROR: > case TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE: > case TCM_UNKNOWN_MODE_PAGE: > case TCM_WRITE_PROTECTED: > @@ -2674,6 +2675,15 @@ transport_send_check_condition_and_sense(struct se_cmd *cmd, > /* INVALID FIELD IN PARAMETER LIST */ > buffer[SPC_ASC_KEY_OFFSET] = 0x26; > break; > + case TCM_PARAMETER_LIST_LENGTH_ERROR: > + /* CURRENT ERROR */ > + buffer[0] = 0x70; > + buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10; > + /* ILLEGAL REQUEST */ > + buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST; > + /* INVALID FIELD IN PARAMETER LIST */ > + buffer[SPC_ASC_KEY_OFFSET] = 0x1a; > + break; > case TCM_UNEXPECTED_UNSOLICITED_DATA: > /* CURRENT ERROR */ > buffer[0] = 0x70; > diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h > index 4fa0f10..37e3baa 100644 > --- a/include/target/target_core_base.h > +++ b/include/target/target_core_base.h > @@ -193,6 +193,7 @@ enum tcm_sense_reason_table { > TCM_RESERVATION_CONFLICT = R(0x10), > TCM_ADDRESS_OUT_OF_RANGE = R(0x11), > TCM_OUT_OF_RESOURCES = R(0x12), > + TCM_PARAMETER_LIST_LENGTH_ERROR = R(0x13), > #undef R > }; > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] target: Fix parameter list length checking in MODE SELECT 2013-02-08 23:18 [PATCH 1/3] target: Fix sense data for out-of-bounds IO operations Roland Dreier 2013-02-08 23:18 ` [PATCH 2/3] target: Fix error checking for UNMAP commands Roland Dreier @ 2013-02-08 23:18 ` Roland Dreier 2013-02-13 20:27 ` Nicholas A. Bellinger 2013-02-13 20:26 ` [PATCH 1/3] target: Fix sense data for out-of-bounds IO operations Nicholas A. Bellinger 2 siblings, 1 reply; 8+ messages in thread From: Roland Dreier @ 2013-02-08 23:18 UTC (permalink / raw) To: Nicholas A. Bellinger; +Cc: target-devel, linux-scsi, Roland Dreier From: Roland Dreier <roland@purestorage.com> An empty parameter list (length == 0) is not an error, so succeed MODE SELECT in this case. If the parameter list length is too small, return the correct sense code of PARAMETER LIST LENGTH ERROR. Signed-off-by: Roland Dreier <roland@purestorage.com> --- drivers/target/target_core_spc.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c index 2d88f08..73c5d53 100644 --- a/drivers/target/target_core_spc.c +++ b/drivers/target/target_core_spc.c @@ -983,6 +983,14 @@ static sense_reason_t spc_emulate_modeselect(struct se_cmd *cmd) int ret = 0; int i; + if (!cmd->data_length) { + target_complete_cmd(cmd, GOOD); + return 0; + } + + if (cmd->data_length < off + 2) + return TCM_PARAMETER_LIST_LENGTH_ERROR; + buf = transport_kmap_data_sg(cmd); if (!buf) return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; @@ -1007,6 +1015,11 @@ static sense_reason_t spc_emulate_modeselect(struct se_cmd *cmd) goto out; check_contents: + if (cmd->data_length < off + length) { + ret = TCM_PARAMETER_LIST_LENGTH_ERROR; + goto out; + } + if (memcmp(buf + off, tbuf, length)) ret = TCM_INVALID_PARAMETER_LIST; -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] target: Fix parameter list length checking in MODE SELECT 2013-02-08 23:18 ` [PATCH 3/3] target: Fix parameter list length checking in MODE SELECT Roland Dreier @ 2013-02-13 20:27 ` Nicholas A. Bellinger 0 siblings, 0 replies; 8+ messages in thread From: Nicholas A. Bellinger @ 2013-02-13 20:27 UTC (permalink / raw) To: Roland Dreier; +Cc: target-devel, linux-scsi, Roland Dreier On Fri, 2013-02-08 at 15:18 -0800, Roland Dreier wrote: > From: Roland Dreier <roland@purestorage.com> > > An empty parameter list (length == 0) is not an error, so succeed MODE > SELECT in this case. If the parameter list length is too small, > return the correct sense code of PARAMETER LIST LENGTH ERROR. > > Signed-off-by: Roland Dreier <roland@purestorage.com> > --- Applied. Thanks Roland! --nab > drivers/target/target_core_spc.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c > index 2d88f08..73c5d53 100644 > --- a/drivers/target/target_core_spc.c > +++ b/drivers/target/target_core_spc.c > @@ -983,6 +983,14 @@ static sense_reason_t spc_emulate_modeselect(struct se_cmd *cmd) > int ret = 0; > int i; > > + if (!cmd->data_length) { > + target_complete_cmd(cmd, GOOD); > + return 0; > + } > + > + if (cmd->data_length < off + 2) > + return TCM_PARAMETER_LIST_LENGTH_ERROR; > + > buf = transport_kmap_data_sg(cmd); > if (!buf) > return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; > @@ -1007,6 +1015,11 @@ static sense_reason_t spc_emulate_modeselect(struct se_cmd *cmd) > goto out; > > check_contents: > + if (cmd->data_length < off + length) { > + ret = TCM_PARAMETER_LIST_LENGTH_ERROR; > + goto out; > + } > + > if (memcmp(buf + off, tbuf, length)) > ret = TCM_INVALID_PARAMETER_LIST; > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] target: Fix sense data for out-of-bounds IO operations 2013-02-08 23:18 [PATCH 1/3] target: Fix sense data for out-of-bounds IO operations Roland Dreier 2013-02-08 23:18 ` [PATCH 2/3] target: Fix error checking for UNMAP commands Roland Dreier 2013-02-08 23:18 ` [PATCH 3/3] target: Fix parameter list length checking in MODE SELECT Roland Dreier @ 2013-02-13 20:26 ` Nicholas A. Bellinger 2 siblings, 0 replies; 8+ messages in thread From: Nicholas A. Bellinger @ 2013-02-13 20:26 UTC (permalink / raw) To: Roland Dreier; +Cc: target-devel, linux-scsi, Roland Dreier Hi Roland, On Fri, 2013-02-08 at 15:18 -0800, Roland Dreier wrote: > From: Roland Dreier <roland@purestorage.com> > > We're supposed to return LOGICAL BLOCK ADDRESS OUT OF RANGE, not > INVALID FIELD IN CDB. > > Signed-off-by: Roland Dreier <roland@purestorage.com> > --- Apologies for the delayed response. Applied to target-pending/for-next. Thank you, --nab > drivers/target/target_core_sbc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c > index a664c66..170f1f7 100644 > --- a/drivers/target/target_core_sbc.c > +++ b/drivers/target/target_core_sbc.c > @@ -486,7 +486,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops) > */ > if (cmd->t_task_lba || sectors) { > if (sbc_check_valid_sectors(cmd) < 0) > - return TCM_INVALID_CDB_FIELD; > + return TCM_ADDRESS_OUT_OF_RANGE; > } > cmd->execute_cmd = ops->execute_sync_cache; > break; ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-02-13 20:27 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-02-08 23:18 [PATCH 1/3] target: Fix sense data for out-of-bounds IO operations Roland Dreier 2013-02-08 23:18 ` [PATCH 2/3] target: Fix error checking for UNMAP commands Roland Dreier 2013-02-09 9:23 ` Chris Boot 2013-02-09 19:27 ` Roland Dreier 2013-02-13 20:27 ` Nicholas A. Bellinger 2013-02-08 23:18 ` [PATCH 3/3] target: Fix parameter list length checking in MODE SELECT Roland Dreier 2013-02-13 20:27 ` Nicholas A. Bellinger 2013-02-13 20:26 ` [PATCH 1/3] target: Fix sense data for out-of-bounds IO operations Nicholas A. Bellinger
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.