[PATCH v2] target: fix truncated PR-in ReadKeys response

[PATCH v2] target: fix truncated PR-in ReadKeys response

[David Disseldorp]

SPC5r17 states that the contents of the ADDITIONAL LENGTH field are not
altered based on the allocation length, so always calculate and pack the
full key list length even if the list itself is truncated.

According to Maged:
  Yes it fixes the "Storage Spaces Persistent Reservation" test in the
  Windows 2016 Server Failover Cluster validation suites when having
  many connections that result in more than 8 registrations. I tested
  your patch on 4.17 with iblock.

This behaviour can be tested using the libiscsi PrinReadKeys.Truncate
test.

Cc: stable@vger.kernel.org
Signed-off-by: David Disseldorp <ddiss@suse.de>
Reviewed-by: Mike Christie <mchristi@redhat.com>
Tested-by: Maged Mokhtar <mmokhtar@petasan.org>
---
 drivers/target/target_core_pr.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Changes since v1:
* CC stable
* mention Maged's Windows PR test fix comment in commit message
* add Reviewed-by and Tested-by tags

diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 01ac306131c1..2e865fdaa362 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -3727,11 +3727,16 @@ core_scsi3_pri_read_keys(struct se_cmd *cmd)
 		 * Check for overflow of 8byte PRI READ_KEYS payload and
 		 * next reservation key list descriptor.
 		 */
-		if ((add_len + 8) > (cmd->data_length - 8))
-			break;
-
-		put_unaligned_be64(pr_reg->pr_res_key, &buf[off]);
-		off += 8;
+		if ((off + 8) <= cmd->data_length) {
+			put_unaligned_be64(pr_reg->pr_res_key, &buf[off]);
+			off += 8;
+		}
+		/*
+		 * SPC5r17: 6.16.2 READ KEYS service action
+		 * The ADDITIONAL LENGTH field indicates the number of bytes in
+		 * the Reservation key list. The contents of the ADDITIONAL
+		 * LENGTH field are not altered based on the allocation length
+		 */
 		add_len += 8;
 	}
 	spin_unlock(&dev->t10_pr.registration_lock);
-- 
2.13.6

Re: [PATCH v2] target: fix truncated PR-in ReadKeys response

[David Disseldorp]

cc'ing the SCSI list, as it doesn't look like anything is making it
through target-devel nowadays...
https://patchwork.kernel.org/patch/10448059/

Cheers, David

On Tue,  5 Jun 2018 12:00:25 +0200, David Disseldorp wrote:

> SPC5r17 states that the contents of the ADDITIONAL LENGTH field are not
> altered based on the allocation length, so always calculate and pack the
> full key list length even if the list itself is truncated.
> 
> According to Maged:
>   Yes it fixes the "Storage Spaces Persistent Reservation" test in the
>   Windows 2016 Server Failover Cluster validation suites when having
>   many connections that result in more than 8 registrations. I tested
>   your patch on 4.17 with iblock.
> 
> This behaviour can be tested using the libiscsi PrinReadKeys.Truncate
> test.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> Reviewed-by: Mike Christie <mchristi@redhat.com>
> Tested-by: Maged Mokhtar <mmokhtar@petasan.org>
> ---
>  drivers/target/target_core_pr.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> Changes since v1:
> * CC stable
> * mention Maged's Windows PR test fix comment in commit message
> * add Reviewed-by and Tested-by tags
> 
> diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
> index 01ac306131c1..2e865fdaa362 100644
> --- a/drivers/target/target_core_pr.c
> +++ b/drivers/target/target_core_pr.c
> @@ -3727,11 +3727,16 @@ core_scsi3_pri_read_keys(struct se_cmd *cmd)
>  		 * Check for overflow of 8byte PRI READ_KEYS payload and
>  		 * next reservation key list descriptor.
>  		 */
> -		if ((add_len + 8) > (cmd->data_length - 8))
> -			break;
> -
> -		put_unaligned_be64(pr_reg->pr_res_key, &buf[off]);
> -		off += 8;
> +		if ((off + 8) <= cmd->data_length) {
> +			put_unaligned_be64(pr_reg->pr_res_key, &buf[off]);
> +			off += 8;
> +		}
> +		/*
> +		 * SPC5r17: 6.16.2 READ KEYS service action
> +		 * The ADDITIONAL LENGTH field indicates the number of bytes in
> +		 * the Reservation key list. The contents of the ADDITIONAL
> +		 * LENGTH field are not altered based on the allocation length
> +		 */
>  		add_len += 8;
>  	}
>  	spin_unlock(&dev->t10_pr.registration_lock);

[PATCH v3] target: fix truncated PR-in ReadKeys response

[David Disseldorp]

SPC5r17 states that the contents of the ADDITIONAL LENGTH field are not
altered based on the allocation length, so always calculate and pack the
full key list length even if the list itself is truncated.

According to Maged:
  Yes it fixes the "Storage Spaces Persistent Reservation" test in the
  Windows 2016 Server Failover Cluster validation suites when having
  many connections that result in more than 8 registrations. I tested
  your patch on 4.17 with iblock.

This behaviour can be tested using the libiscsi PrinReadKeys.Truncate
test.

Cc: stable@vger.kernel.org
Signed-off-by: David Disseldorp <ddiss@suse.de>
Reviewed-by: Mike Christie <mchristi@redhat.com>
Tested-by: Maged Mokhtar <mmokhtar@petasan.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
Changes since v2:
* drop unnecessary braces
* add Christoph's Reviewed-by

Changes since v1:
* CC stable
* mention Maged's Windows PR test fix comment in commit message
* add Reviewed-by and Tested-by tags

 drivers/target/target_core_pr.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 01ac306131c1..10db5656fd5d 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -3727,11 +3727,16 @@ core_scsi3_pri_read_keys(struct se_cmd *cmd)
 		 * Check for overflow of 8byte PRI READ_KEYS payload and
 		 * next reservation key list descriptor.
 		 */
-		if ((add_len + 8) > (cmd->data_length - 8))
-			break;
-
-		put_unaligned_be64(pr_reg->pr_res_key, &buf[off]);
-		off += 8;
+		if (off + 8 <= cmd->data_length) {
+			put_unaligned_be64(pr_reg->pr_res_key, &buf[off]);
+			off += 8;
+		}
+		/*
+		 * SPC5r17: 6.16.2 READ KEYS service action
+		 * The ADDITIONAL LENGTH field indicates the number of bytes in
+		 * the Reservation key list. The contents of the ADDITIONAL
+		 * LENGTH field are not altered based on the allocation length
+		 */
 		add_len += 8;
 	}
 	spin_unlock(&dev->t10_pr.registration_lock);
-- 
2.13.7

Re: [PATCH v3] target: fix truncated PR-in ReadKeys response

[Martin K. Petersen]

David,

> SPC5r17 states that the contents of the ADDITIONAL LENGTH field are not
> altered based on the allocation length, so always calculate and pack the
> full key list length even if the list itself is truncated.
>
> According to Maged:
>   Yes it fixes the "Storage Spaces Persistent Reservation" test in the
>   Windows 2016 Server Failover Cluster validation suites when having
>   many connections that result in more than 8 registrations. I tested
>   your patch on 4.17 with iblock.
>
> This behaviour can be tested using the libiscsi PrinReadKeys.Truncate
> test.

Applied to 4.18/scsi-fixes. Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2] target: fix truncated PR-in ReadKeys response

[Christoph Hellwig]

> +		if ((off + 8) <= cmd->data_length) {

No need for the inner braces.

> +			put_unaligned_be64(pr_reg->pr_res_key, &buf[off]);
> +			off += 8;
> +		}
> +		/*
> +		 * SPC5r17: 6.16.2 READ KEYS service action
> +		 * The ADDITIONAL LENGTH field indicates the number of bytes in
> +		 * the Reservation key list. The contents of the ADDITIONAL
> +		 * LENGTH field are not altered based on the allocation length
> +		 */
>  		add_len += 8;
>  	}
>  	spin_unlock(&dev->t10_pr.registration_lock);

Otherwise looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>