[PATCH 0/2] Fix target not properly truncating command data length

[PATCH 0/2] Fix target not properly truncating command data length

[Aleksandr Miloserdov]

SPC-5 (4.2.5.6 Allocation length) requires to terminate transfers to the
Data In Buffer when the number of bytes or blocks specified by the
ALLOCATION LENGTH field have been transferred or when all available data
have been transferred, whichever is less.

PERSISTENT RESERVE IN service actions in TCM don't follow the clause and
return ALLOCATION LENGTH of data, even if actual number of data in reply
is less (e.g. there are no reservation keys).

That causes an underflow and a failure in libiscsi PrinReadKeys.Simple
that expects Data In Buffer size equal to ADDITIONAL LENGTH + 8.

This patch series fixes this behavior.
It is intended for 5.11/scsi-queue branch.

Aleksandr Miloserdov (2):
  scsi: target: core: Add cmd length set before cmd complete
  scsi: target: core: Prevent underflow for service actions

 drivers/target/target_core_pr.c        |  6 ++++++
 drivers/target/target_core_transport.c | 15 +++++++++++----
 include/target/target_core_backend.h   |  1 +
 3 files changed, 18 insertions(+), 4 deletions(-)

-- 
2.26.2

[PATCH 1/2] scsi: target: core: Add cmd length set before cmd complete

[Aleksandr Miloserdov]

TCM doesn't properly handle underflow case for service actions. One way to
prevent it is to always complete command with
target_complete_cmd_with_length, however it requires access to data_sg,
which is not always available.

This change introduces target_set_cmd_data_length function which allows to
set command data length before completing it.

Signed-off-by: Aleksandr Miloserdov <a.miloserdov@yadro.com>
Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 drivers/target/target_core_transport.c | 15 +++++++++++----
 include/target/target_core_backend.h   |  1 +
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index fca4bd079d02..c98540355512 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -879,11 +879,9 @@ void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status)
 }
 EXPORT_SYMBOL(target_complete_cmd);
 
-void target_complete_cmd_with_length(struct se_cmd *cmd, u8 scsi_status, int length)
+void target_set_cmd_data_length(struct se_cmd *cmd, int length)
 {
-	if ((scsi_status == SAM_STAT_GOOD ||
-	     cmd->se_cmd_flags & SCF_TREAT_READ_AS_NORMAL) &&
-	    length < cmd->data_length) {
+	if (length < cmd->data_length) {
 		if (cmd->se_cmd_flags & SCF_UNDERFLOW_BIT) {
 			cmd->residual_count += cmd->data_length - length;
 		} else {
@@ -893,6 +891,15 @@ void target_complete_cmd_with_length(struct se_cmd *cmd, u8 scsi_status, int len
 
 		cmd->data_length = length;
 	}
+}
+EXPORT_SYMBOL(target_set_cmd_data_length);
+
+void target_complete_cmd_with_length(struct se_cmd *cmd, u8 scsi_status, int length)
+{
+	if (scsi_status == SAM_STAT_GOOD ||
+	    cmd->se_cmd_flags & SCF_TREAT_READ_AS_NORMAL) {
+		target_set_cmd_data_length(cmd, length);
+	}
 
 	target_complete_cmd(cmd, scsi_status);
 }
diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
index 6336780d83a7..ce2fba49c95d 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -72,6 +72,7 @@ int	transport_backend_register(const struct target_backend_ops *);
 void	target_backend_unregister(const struct target_backend_ops *);
 
 void	target_complete_cmd(struct se_cmd *, u8);
+void	target_set_cmd_data_length(struct se_cmd *, int);
 void	target_complete_cmd_with_length(struct se_cmd *, u8, int);
 
 void	transport_copy_sense_to_cmd(struct se_cmd *, unsigned char *);
-- 
2.26.2

Re: [PATCH 1/2] scsi: target: core: Add cmd length set before cmd complete

[Bodo Stroesser]

On 09.02.21 08:22, Aleksandr Miloserdov wrote:
> TCM doesn't properly handle underflow case for service actions. One way to
> prevent it is to always complete command with
> target_complete_cmd_with_length, however it requires access to data_sg,
> which is not always available.
> 
> This change introduces target_set_cmd_data_length function which allows to
> set command data length before completing it.
> 
> Signed-off-by: Aleksandr Miloserdov <a.miloserdov@yadro.com>
> Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---
>   drivers/target/target_core_transport.c | 15 +++++++++++----
>   include/target/target_core_backend.h   |  1 +
>   2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index fca4bd079d02..c98540355512 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -879,11 +879,9 @@ void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status)
>   }
>   EXPORT_SYMBOL(target_complete_cmd);
>   
> -void target_complete_cmd_with_length(struct se_cmd *cmd, u8 scsi_status, int length)
> +void target_set_cmd_data_length(struct se_cmd *cmd, int length)
>   {
> -	if ((scsi_status == SAM_STAT_GOOD ||
> -	     cmd->se_cmd_flags & SCF_TREAT_READ_AS_NORMAL) &&
> -	    length < cmd->data_length) {
> +	if (length < cmd->data_length) {
>   		if (cmd->se_cmd_flags & SCF_UNDERFLOW_BIT) {
>   			cmd->residual_count += cmd->data_length - length;
>   		} else {
> @@ -893,6 +891,15 @@ void target_complete_cmd_with_length(struct se_cmd *cmd, u8 scsi_status, int len
>   
>   		cmd->data_length = length;
>   	}
> +}
> +EXPORT_SYMBOL(target_set_cmd_data_length);
> +
> +void target_complete_cmd_with_length(struct se_cmd *cmd, u8 scsi_status, int length)
> +{
> +	if (scsi_status == SAM_STAT_GOOD ||
> +	    cmd->se_cmd_flags & SCF_TREAT_READ_AS_NORMAL) {
> +		target_set_cmd_data_length(cmd, length);
> +	}
>   
>   	target_complete_cmd(cmd, scsi_status);
>   }
> diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
> index 6336780d83a7..ce2fba49c95d 100644
> --- a/include/target/target_core_backend.h
> +++ b/include/target/target_core_backend.h
> @@ -72,6 +72,7 @@ int	transport_backend_register(const struct target_backend_ops *);
>   void	target_backend_unregister(const struct target_backend_ops *);
>   
>   void	target_complete_cmd(struct se_cmd *, u8);
> +void	target_set_cmd_data_length(struct se_cmd *, int);
>   void	target_complete_cmd_with_length(struct se_cmd *, u8, int);
>   
>   void	transport_copy_sense_to_cmd(struct se_cmd *, unsigned char *);
> 

Looks ok to me.

Reviewed-by: Bodo Stroesser <bostroesser@gmail.com>

[PATCH 2/2] scsi: target: core: Prevent underflow for service actions

[Aleksandr Miloserdov]

TCM buffer length doesn't necessarily equal 8 + ADDITIONAL LENGTH which
might be considered an underflow in case of Data-In size being greater than
8 + ADDITIONAL LENGTH. So truncate buffer length to prevent underflow.

Signed-off-by: Aleksandr Miloserdov <a.miloserdov@yadro.com>
Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 drivers/target/target_core_pr.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 14db5e568f22..a13140e95b47 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -3739,6 +3739,7 @@ core_scsi3_pri_read_keys(struct se_cmd *cmd)
 	spin_unlock(&dev->t10_pr.registration_lock);
 
 	put_unaligned_be32(add_len, &buf[4]);
+	target_set_cmd_data_length(cmd, 8 + add_len);
 
 	transport_kunmap_data_sg(cmd);
 
@@ -3757,7 +3758,7 @@ core_scsi3_pri_read_reservation(struct se_cmd *cmd)
 	struct t10_pr_registration *pr_reg;
 	unsigned char *buf;
 	u64 pr_res_key;
-	u32 add_len = 16; /* Hardcoded to 16 when a reservation is held. */
+	u32 add_len = 0;
 
 	if (cmd->data_length < 8) {
 		pr_err("PRIN SA READ_RESERVATIONS SCSI Data Length: %u"
@@ -3775,8 +3776,9 @@ core_scsi3_pri_read_reservation(struct se_cmd *cmd)
 	pr_reg = dev->dev_pr_res_holder;
 	if (pr_reg) {
 		/*
-		 * Set the hardcoded Additional Length
+		 * Set the Additional Length to 16 when a reservation is held
 		 */
+		add_len = 16;
 		put_unaligned_be32(add_len, &buf[4]);
 
 		if (cmd->data_length < 22)
@@ -3812,6 +3814,8 @@ core_scsi3_pri_read_reservation(struct se_cmd *cmd)
 			  (pr_reg->pr_res_type & 0x0f);
 	}
 
+	target_set_cmd_data_length(cmd, 8 + add_len);
+
 err:
 	spin_unlock(&dev->dev_reservation_lock);
 	transport_kunmap_data_sg(cmd);
@@ -3830,7 +3834,7 @@ core_scsi3_pri_report_capabilities(struct se_cmd *cmd)
 	struct se_device *dev = cmd->se_dev;
 	struct t10_reservation *pr_tmpl = &dev->t10_pr;
 	unsigned char *buf;
-	u16 add_len = 8; /* Hardcoded to 8. */
+	u16 len = 8; /* Hardcoded to 8. */
 
 	if (cmd->data_length < 6) {
 		pr_err("PRIN SA REPORT_CAPABILITIES SCSI Data Length:"
@@ -3842,7 +3846,7 @@ core_scsi3_pri_report_capabilities(struct se_cmd *cmd)
 	if (!buf)
 		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 
-	put_unaligned_be16(add_len, &buf[0]);
+	put_unaligned_be16(len, &buf[0]);
 	buf[2] |= 0x10; /* CRH: Compatible Reservation Hanlding bit. */
 	buf[2] |= 0x08; /* SIP_C: Specify Initiator Ports Capable bit */
 	buf[2] |= 0x04; /* ATP_C: All Target Ports Capable bit */
@@ -3871,6 +3875,8 @@ core_scsi3_pri_report_capabilities(struct se_cmd *cmd)
 	buf[4] |= 0x02; /* PR_TYPE_WRITE_EXCLUSIVE */
 	buf[5] |= 0x01; /* PR_TYPE_EXCLUSIVE_ACCESS_ALLREG */
 
+	target_set_cmd_data_length(cmd, len);
+
 	transport_kunmap_data_sg(cmd);
 
 	return 0;
@@ -4031,6 +4037,7 @@ core_scsi3_pri_read_full_status(struct se_cmd *cmd)
 	 * Set ADDITIONAL_LENGTH
 	 */
 	put_unaligned_be32(add_len, &buf[4]);
+	target_set_cmd_data_length(cmd, 8 + add_len);
 
 	transport_kunmap_data_sg(cmd);
 
-- 
2.28.0

Re: [PATCH 2/2] scsi: target: core: Prevent underflow for service actions

[Bodo Stroesser]

On 09.02.21 08:22, Aleksandr Miloserdov wrote:
> TCM buffer length doesn't necessarily equal 8 + ADDITIONAL LENGTH which
> might be considered an underflow in case of Data-In size being greater than
> 8 + ADDITIONAL LENGTH. So truncate buffer length to prevent underflow.
> 
> Signed-off-by: Aleksandr Miloserdov <a.miloserdov@yadro.com>
> Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---
>   drivers/target/target_core_pr.c | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
> index 14db5e568f22..a13140e95b47 100644
> --- a/drivers/target/target_core_pr.c
> +++ b/drivers/target/target_core_pr.c
> @@ -3739,6 +3739,7 @@ core_scsi3_pri_read_keys(struct se_cmd *cmd)
>   	spin_unlock(&dev->t10_pr.registration_lock);
>   
>   	put_unaligned_be32(add_len, &buf[4]);
> +	target_set_cmd_data_length(cmd, 8 + add_len);
>   
>   	transport_kunmap_data_sg(cmd);
>   
> @@ -3757,7 +3758,7 @@ core_scsi3_pri_read_reservation(struct se_cmd *cmd)
>   	struct t10_pr_registration *pr_reg;
>   	unsigned char *buf;
>   	u64 pr_res_key;
> -	u32 add_len = 16; /* Hardcoded to 16 when a reservation is held. */
> +	u32 add_len = 0;
>   
>   	if (cmd->data_length < 8) {
>   		pr_err("PRIN SA READ_RESERVATIONS SCSI Data Length: %u"
> @@ -3775,8 +3776,9 @@ core_scsi3_pri_read_reservation(struct se_cmd *cmd)
>   	pr_reg = dev->dev_pr_res_holder;
>   	if (pr_reg) {
>   		/*
> -		 * Set the hardcoded Additional Length
> +		 * Set the Additional Length to 16 when a reservation is held
>   		 */
> +		add_len = 16;
>   		put_unaligned_be32(add_len, &buf[4]);
>   
>   		if (cmd->data_length < 22)
> @@ -3812,6 +3814,8 @@ core_scsi3_pri_read_reservation(struct se_cmd *cmd)
>   			  (pr_reg->pr_res_type & 0x0f);
>   	}
>   
> +	target_set_cmd_data_length(cmd, 8 + add_len);
> +
>   err:
>   	spin_unlock(&dev->dev_reservation_lock);
>   	transport_kunmap_data_sg(cmd);
> @@ -3830,7 +3834,7 @@ core_scsi3_pri_report_capabilities(struct se_cmd *cmd)
>   	struct se_device *dev = cmd->se_dev;
>   	struct t10_reservation *pr_tmpl = &dev->t10_pr;
>   	unsigned char *buf;
> -	u16 add_len = 8; /* Hardcoded to 8. */
> +	u16 len = 8; /* Hardcoded to 8. */
>   
>   	if (cmd->data_length < 6) {
>   		pr_err("PRIN SA REPORT_CAPABILITIES SCSI Data Length:"
> @@ -3842,7 +3846,7 @@ core_scsi3_pri_report_capabilities(struct se_cmd *cmd)
>   	if (!buf)
>   		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>   
> -	put_unaligned_be16(add_len, &buf[0]);
> +	put_unaligned_be16(len, &buf[0]);
>   	buf[2] |= 0x10; /* CRH: Compatible Reservation Hanlding bit. */
>   	buf[2] |= 0x08; /* SIP_C: Specify Initiator Ports Capable bit */
>   	buf[2] |= 0x04; /* ATP_C: All Target Ports Capable bit */
> @@ -3871,6 +3875,8 @@ core_scsi3_pri_report_capabilities(struct se_cmd *cmd)
>   	buf[4] |= 0x02; /* PR_TYPE_WRITE_EXCLUSIVE */
>   	buf[5] |= 0x01; /* PR_TYPE_EXCLUSIVE_ACCESS_ALLREG */
>   
> +	target_set_cmd_data_length(cmd, len);
> +
>   	transport_kunmap_data_sg(cmd);
>   
>   	return 0;
> @@ -4031,6 +4037,7 @@ core_scsi3_pri_read_full_status(struct se_cmd *cmd)
>   	 * Set ADDITIONAL_LENGTH
>   	 */
>   	put_unaligned_be32(add_len, &buf[4]);
> +	target_set_cmd_data_length(cmd, 8 + add_len);
>   
>   	transport_kunmap_data_sg(cmd);
>   
> 


Looks ok to me.

Reviewed-by: Bodo Stroesser <bostroesser@gmail.com>

Re: [PATCH 0/2] Fix target not properly truncating command data length

[Martin K. Petersen]

Aleksandr,

> SPC-5 (4.2.5.6 Allocation length) requires to terminate transfers to
> the Data In Buffer when the number of bytes or blocks specified by the
> ALLOCATION LENGTH field have been transferred or when all available
> data have been transferred, whichever is less.
>
> PERSISTENT RESERVE IN service actions in TCM don't follow the clause
> and return ALLOCATION LENGTH of data, even if actual number of data in
> reply is less (e.g. there are no reservation keys).
>
> That causes an underflow and a failure in libiscsi PrinReadKeys.Simple
> that expects Data In Buffer size equal to ADDITIONAL LENGTH + 8.

Applied to 5.12/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 0/2] Fix target not properly truncating command data length

[Martin K. Petersen]

On Tue, 9 Feb 2021 10:22:00 +0300, Aleksandr Miloserdov wrote:

> SPC-5 (4.2.5.6 Allocation length) requires to terminate transfers to the
> Data In Buffer when the number of bytes or blocks specified by the
> ALLOCATION LENGTH field have been transferred or when all available data
> have been transferred, whichever is less.
> 
> PERSISTENT RESERVE IN service actions in TCM don't follow the clause and
> return ALLOCATION LENGTH of data, even if actual number of data in reply
> is less (e.g. there are no reservation keys).
> 
> [...]

Applied to 5.12/scsi-queue, thanks!

[1/2] scsi: target: core: Add cmd length set before cmd complete
      https://git.kernel.org/mkp/scsi/c/1c73e0c5e54d
[2/2] scsi: target: core: Prevent underflow for service actions
      https://git.kernel.org/mkp/scsi/c/14d24e2cc774

-- 
Martin K. Petersen	Oracle Linux Engineering