mbox series

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

Message ID 20210209072202.41154-1-a.miloserdov@yadro.com
Headers show
Series Fix target not properly truncating command data length | expand

Message

Aleksandr Miloserdov Feb. 9, 2021, 7:22 a.m. UTC 
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(-)

Comments

Bodo Stroesser Feb. 9, 2021, 5:30 p.m. UTC | #1 
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>
Martin K. Petersen Feb. 23, 2021, 3:22 a.m. UTC | #2 
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
Martin K. Petersen Feb. 26, 2021, 2:22 a.m. UTC | #3 
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