[PATCH 0/3] Descriptor format sense data

[PATCH 0/3] Descriptor format sense data

[Sagi Grimberg]

This small patchset addresses some issues with descriptor format
sense data.

Patch 1 fixes wrong additional sense length for descriptor format
information field.

Patch 2 protects against buffer overflow in scsi_set_information_sense
(reported by Hannes).

Patch 3 converts the target stack to report descriptor format sense
data to avoid 64bit sector info truncation (reported by hch).

Sagi Grimberg (3):
  scsi: Fix wrong additional sense length in descriptor format
  scsi: Protect against buffer possible overflow in
    scsi_set_sense_information
  target: Return descriptor format sense data

 drivers/ata/libata-scsi.c              |  4 +++-
 drivers/scsi/scsi_common.c             | 15 +++++++++++++--
 drivers/target/target_core_spc.c       | 11 ++++++++---
 drivers/target/target_core_transport.c | 16 ++++++++++++----
 include/scsi/scsi_common.h             |  2 +-
 5 files changed, 37 insertions(+), 11 deletions(-)

-- 
1.8.4.3

[PATCH 1/3] scsi: Fix wrong additional sense length in descriptor format

[Sagi Grimberg]

The sense header additional sense length should be the accumulated
size of all the descriptors. Information descriptor size is 12 bytes.
When setting the additional sense length we should add 0xc instead of
0xa.

Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
---
 drivers/scsi/scsi_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_common.c b/drivers/scsi/scsi_common.c
index 8cfb7ee..fbf137b 100644
--- a/drivers/scsi/scsi_common.c
+++ b/drivers/scsi/scsi_common.c
@@ -260,7 +260,7 @@ void scsi_set_sense_information(u8 *buf, u64 info)
 		len = buf[7];
 		ucp = (char *)scsi_sense_desc_find(buf, len + 8, 0);
 		if (!ucp) {
-			buf[7] = len + 0xa;
+			buf[7] = len + 0xc;
 			ucp = buf + 8 + len;
 		}
 		ucp[0] = 0;
-- 
1.8.4.3

Re: [PATCH 1/3] scsi: Fix wrong additional sense length in descriptor format

[Hannes Reinecke]

On 07/12/2015 12:37 PM, Sagi Grimberg wrote:
> The sense header additional sense length should be the accumulated
> size of all the descriptors. Information descriptor size is 12 bytes.
> When setting the additional sense length we should add 0xc instead of
> 0xa.
> 
> Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
> ---
>  drivers/scsi/scsi_common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_common.c b/drivers/scsi/scsi_common.c
> index 8cfb7ee..fbf137b 100644
> --- a/drivers/scsi/scsi_common.c
> +++ b/drivers/scsi/scsi_common.c
> @@ -260,7 +260,7 @@ void scsi_set_sense_information(u8 *buf, u64 info)
>  		len = buf[7];
>  		ucp = (char *)scsi_sense_desc_find(buf, len + 8, 0);
>  		if (!ucp) {
> -			buf[7] = len + 0xa;
> +			buf[7] = len + 0xc;
>  			ucp = buf + 8 + len;
>  		}
>  		ucp[0] = 0;
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 1/3] scsi: Fix wrong additional sense length in descriptor format

[Martin K. Petersen]

>>>>> "Sagi" == Sagi Grimberg <sagig@mellanox.com> writes:

Sagi> The sense header additional sense length should be the accumulated
Sagi> size of all the descriptors. Information descriptor size is 12
Sagi> bytes.  When setting the additional sense length we should add 0xc
Sagi> instead of 0xa.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH 2/3] scsi: Protect against buffer possible overflow in scsi_set_sense_information

[Sagi Grimberg]

Make sure that the input sense buffer has sufficient length
to fit the information descriptor (12 additional bytes).
Modify scsi_set_sense_information to receive the sense buffer
length and adjust it's callers scsi target and libata.

Reported-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
Cc: Tejun Heo <tj@kernel.org>
---
 drivers/ata/libata-scsi.c              |  4 +++-
 drivers/scsi/scsi_common.c             | 13 ++++++++++++-
 drivers/target/target_core_transport.c | 14 +++++++++++---
 include/scsi/scsi_common.h             |  2 +-
 4 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 3131adc..2fb7c79 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -289,7 +289,9 @@ void ata_scsi_set_sense_information(struct scsi_cmnd *cmd,
 		return;
 
 	information = ata_tf_read_block(tf, NULL);
-	scsi_set_sense_information(cmd->sense_buffer, information);
+	scsi_set_sense_information(cmd->sense_buffer,
+				   SCSI_SENSE_BUFFERSIZE,
+				   information);
 }
 
 static ssize_t
diff --git a/drivers/scsi/scsi_common.c b/drivers/scsi/scsi_common.c
index fbf137b..2b27e64 100644
--- a/drivers/scsi/scsi_common.c
+++ b/drivers/scsi/scsi_common.c
@@ -5,6 +5,7 @@
 #include <linux/bug.h>
 #include <linux/kernel.h>
 #include <linux/string.h>
+#include <linux/errno.h>
 #include <asm/unaligned.h>
 #include <scsi/scsi_common.h>
 
@@ -249,10 +250,13 @@ EXPORT_SYMBOL(scsi_build_sense_buffer);
  * scsi_set_sense_information - set the information field in a
  *		formatted sense data buffer
  * @buf:	Where to build sense data
+ * @buf_len:    buffer length
  * @info:	64-bit information value to be set
  *
+ * Return value:
+ *	0 on success or EINVAL for invalid sense buffer length
  **/
-void scsi_set_sense_information(u8 *buf, u64 info)
+int scsi_set_sense_information(u8 *buf, int buf_len, u64 info)
 {
 	if ((buf[0] & 0x7f) == 0x72) {
 		u8 *ucp, len;
@@ -263,6 +267,11 @@ void scsi_set_sense_information(u8 *buf, u64 info)
 			buf[7] = len + 0xc;
 			ucp = buf + 8 + len;
 		}
+
+		if (buf_len < len + 0xc)
+			/* Not enough room for info */
+			return -EINVAL;
+
 		ucp[0] = 0;
 		ucp[1] = 0xa;
 		ucp[2] = 0x80; /* Valid bit */
@@ -272,5 +281,7 @@ void scsi_set_sense_information(u8 *buf, u64 info)
 		buf[0] |= 0x80;
 		put_unaligned_be32(info, &buf[3]);
 	}
+
+	return 0;
 }
 EXPORT_SYMBOL(scsi_set_sense_information);
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 2bece60..7fb031b 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2729,7 +2729,7 @@ static const struct sense_info sense_info_table[] = {
 	},
 };
 
-static void translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason)
+static int translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason)
 {
 	const struct sense_info *si;
 	u8 *buffer = cmd->sense_buffer;
@@ -2756,7 +2756,11 @@ static void translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason)
 
 	scsi_build_sense_buffer(0, buffer, si->key, asc, ascq);
 	if (si->add_sector_info)
-		scsi_set_sense_information(buffer, cmd->bad_sector);
+		return scsi_set_sense_information(buffer,
+						  cmd->scsi_sense_length,
+						  cmd->bad_sector);
+
+	return 0;
 }
 
 int
@@ -2774,10 +2778,14 @@ transport_send_check_condition_and_sense(struct se_cmd *cmd,
 	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 
 	if (!from_transport) {
+		int rc;
+
 		cmd->se_cmd_flags |= SCF_EMULATED_TASK_SENSE;
-		translate_sense_reason(cmd, reason);
 		cmd->scsi_status = SAM_STAT_CHECK_CONDITION;
 		cmd->scsi_sense_length  = TRANSPORT_SENSE_BUFFER;
+		rc = translate_sense_reason(cmd, reason);
+		if (rc)
+			return rc;
 	}
 
 	trace_target_cmd_complete(cmd);
diff --git a/include/scsi/scsi_common.h b/include/scsi/scsi_common.h
index 156d673..11571b2 100644
--- a/include/scsi/scsi_common.h
+++ b/include/scsi/scsi_common.h
@@ -62,7 +62,7 @@ extern bool scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
 				 struct scsi_sense_hdr *sshdr);
 
 extern void scsi_build_sense_buffer(int desc, u8 *buf, u8 key, u8 asc, u8 ascq);
-extern void scsi_set_sense_information(u8 *buf, u64 info);
+int scsi_set_sense_information(u8 *buf, int buf_len, u64 info);
 extern const u8 * scsi_sense_desc_find(const u8 * sense_buffer, int sb_len,
 				       int desc_type);
 
-- 
1.8.4.3

Re: [PATCH 2/3] scsi: Protect against buffer possible overflow in scsi_set_sense_information

[Martin K. Petersen]

>>>>> "Sagi" == Sagi Grimberg <sagig@mellanox.com> writes:

Sagi> Make sure that the input sense buffer has sufficient length to fit
Sagi> the information descriptor (12 additional bytes).  Modify
Sagi> scsi_set_sense_information to receive the sense buffer length and
Sagi> adjust it's callers scsi target and libata.

Nitpicking: s/it's/its/

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH 3/3] target: Return descriptor format sense data

[Sagi Grimberg]

Fixed size sense data information field is only 32 bits which
means the sector (64 bits) information will be truncated.

Move to descriptor format sense data to correctly report full
sector information.

Reported-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
---
 drivers/target/target_core_spc.c       | 11 ++++++++---
 drivers/target/target_core_transport.c |  2 +-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index c43dcbf..2899675 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -761,7 +761,12 @@ static int spc_modesense_control(struct se_cmd *cmd, u8 pc, u8 *p)
 	if (pc == 1)
 		goto out;
 
-	p[2] = 2;
+	/*
+	 * GLTSD:   No implicit save of log parameters
+	 * D_SENSE: Descriptor format sense data
+	 */
+	p[2] = (1 << 1 | 1 << 2);
+
 	/*
 	 * From spc4r23, 7.4.7 Control mode page
 	 *
@@ -1158,10 +1163,10 @@ static sense_reason_t spc_emulate_request_sense(struct se_cmd *cmd)
 		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 
 	if (!core_scsi3_ua_clear_for_request_sense(cmd, &ua_asc, &ua_ascq))
-		scsi_build_sense_buffer(0, buf, UNIT_ATTENTION,
+		scsi_build_sense_buffer(1, buf, UNIT_ATTENTION,
 					ua_asc, ua_ascq);
 	else
-		scsi_build_sense_buffer(0, buf, NO_SENSE, 0x0, 0x0);
+		scsi_build_sense_buffer(1, buf, NO_SENSE, 0x0, 0x0);
 
 	memcpy(rbuf, buf, min_t(u32, sizeof(buf), cmd->data_length));
 	transport_kunmap_data_sg(cmd);
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 7fb031b..935296c 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2754,7 +2754,7 @@ static int translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason)
 		ascq = si->ascq;
 	}
 
-	scsi_build_sense_buffer(0, buffer, si->key, asc, ascq);
+	scsi_build_sense_buffer(1, buffer, si->key, asc, ascq);
 	if (si->add_sector_info)
 		return scsi_set_sense_information(buffer,
 						  cmd->scsi_sense_length,
-- 
1.8.4.3

Re: [PATCH 3/3] target: Return descriptor format sense data

[Martin K. Petersen]

>>>>> "Sagi" == Sagi Grimberg <sagig@mellanox.com> writes:

Sagi> Fixed size sense data information field is only 32 bits which
Sagi> means the sector (64 bits) information will be truncated.

Sagi> Move to descriptor format sense data to correctly report full
Sagi> sector information.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 3/3] target: Return descriptor format sense data

[Christoph Hellwig]

On Sun, Jul 12, 2015 at 01:37:03PM +0300, Sagi Grimberg wrote:
> Fixed size sense data information field is only 32 bits which
> means the sector (64 bits) information will be truncated.
> 
> Move to descriptor format sense data to correctly report full
> sector information.

I think this needs to be a tunable as old initiators might not be
able to cope with descriptor sense data.  My idea was to only turn
it own if the LU is large enough to need it.  Initiators that can
deal with large LUs by using READ CAPACITY (16) and READ/WRITE (16)
should be able to handle descriptor style sense data as well.

Re: [PATCH 3/3] target: Return descriptor format sense data

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@infradead.org> writes:

Christoph> I think this needs to be a tunable as old initiators might
Christoph> not be able to cope with descriptor sense data.  My idea was
Christoph> to only turn it own if the LU is large enough to need it.

We could make it conditional and only use the descriptor format if the
LBA is big enough to warrant it.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 3/3] target: Return descriptor format sense data

[Sagi Grimberg]

On 7/14/2015 11:21 AM, Martin K. Petersen wrote:
>>>>>> "Christoph" == Christoph Hellwig <hch@infradead.org> writes:
>
> Christoph> I think this needs to be a tunable as old initiators might
> Christoph> not be able to cope with descriptor sense data.  My idea was
> Christoph> to only turn it own if the LU is large enough to need it.
>
> We could make it conditional and only use the descriptor format if the
> LBA is big enough to warrant it.
>

So would this be acceptable?

target: Return descriptor format sense data in case the LU spans 64bit 
sectors

In case a LU spans 64bit sectors, fixed size sense data information
field is only 32 bits which means the sector information will be truncated.

Thus, if the LU spans 64bit sectors, use descriptor format sense data to
correctly report sector information.

Reported-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
---
  drivers/target/target_core_spc.c       |   12 +++++++++---
  drivers/target/target_core_transport.c |    3 ++-
  include/target/target_core_backend.h   |    3 +++
  3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_spc.c 
b/drivers/target/target_core_spc.c
index c43dcbf..f66abc1 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -761,7 +761,12 @@ static int spc_modesense_control(struct se_cmd 
*cmd, u8 pc, u8 *p)
         if (pc == 1)
                 goto out;

-       p[2] = 2;
+       /* GLTSD: No implicit save of log parameters */
+       p[2] = (1 << 1);
+       if (TARGET_SENSE_DESC_FORMAT(dev))
+               /* D_SENSE: Descriptor format sense data for 64bit 
sectors */
+               p[2] |= (1 << 2);
+
         /*
          * From spc4r23, 7.4.7 Control mode page
          *
@@ -1144,6 +1149,7 @@ static sense_reason_t 
spc_emulate_request_sense(struct se_cmd *cmd)
         unsigned char *rbuf;
         u8 ua_asc = 0, ua_ascq = 0;
         unsigned char buf[SE_SENSE_BUF];
+       bool desc_format = TARGET_SENSE_DESC_FORMAT(cmd->se_dev);

         memset(buf, 0, SE_SENSE_BUF);

@@ -1158,10 +1164,10 @@ static sense_reason_t 
spc_emulate_request_sense(struct se_cmd *cmd)
                 return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;

         if (!core_scsi3_ua_clear_for_request_sense(cmd, &ua_asc, &ua_ascq))
-               scsi_build_sense_buffer(0, buf, UNIT_ATTENTION,
+               scsi_build_sense_buffer(desc_format, buf, UNIT_ATTENTION,
                                         ua_asc, ua_ascq);
         else
-               scsi_build_sense_buffer(0, buf, NO_SENSE, 0x0, 0x0);
+               scsi_build_sense_buffer(desc_format, buf, NO_SENSE, 0x0, 
0x0);

         memcpy(rbuf, buf, min_t(u32, sizeof(buf), cmd->data_length));
         transport_kunmap_data_sg(cmd);
diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index 7fb031b..238c778 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2735,6 +2735,7 @@ static int translate_sense_reason(struct se_cmd 
*cmd, sense_reason_t reason)
         u8 *buffer = cmd->sense_buffer;
         int r = (__force int)reason;
         u8 asc, ascq;
+       bool desc_format = TARGET_SENSE_DESC_FORMAT(cmd->se_dev);

         if (r < ARRAY_SIZE(sense_info_table) && sense_info_table[r].key)
                 si = &sense_info_table[r];
@@ -2754,7 +2755,7 @@ static int translate_sense_reason(struct se_cmd 
*cmd, sense_reason_t reason)
                 ascq = si->ascq;
         }

-       scsi_build_sense_buffer(0, buffer, si->key, asc, ascq);
+       scsi_build_sense_buffer(desc_format, buffer, si->key, asc, ascq);
         if (si->add_sector_info)
                 return scsi_set_sense_information(buffer,
                                                   cmd->scsi_sense_length,
diff --git a/include/target/target_core_backend.h 
b/include/target/target_core_backend.h
index 1e5c8f9..6ce370f 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -3,6 +3,9 @@

  #define TRANSPORT_FLAG_PASSTHROUGH             1

+#define TARGET_SENSE_DESC_FORMAT(dev)          \
+       dev->transport->get_blocks(dev) >= 0xffffffffULL
+
  struct target_backend_ops {
         char name[16];
         char inquiry_prod[16];
--

Re: [PATCH 3/3] target: Return descriptor format sense data

[Bart Van Assche]

On 07/14/15 04:59, Sagi Grimberg wrote:
> +#define TARGET_SENSE_DESC_FORMAT(dev)          \
> +       dev->transport->get_blocks(dev) >= 0xffffffffULL

 From SPC-5: "4.4.4 Returning a value in the INFORMATION field in the 
sense data. To return a value less than or equal to FFFF_FFFFh in the 
INFORMATION field: [ ... ]". I think this means the value FFFF_FFFFh is 
allowed in fixed format sense data.

Bart.

Re: [PATCH 3/3] target: Return descriptor format sense data

[Christoph Hellwig]

On Tue, Jul 14, 2015 at 02:59:51PM +0300, Sagi Grimberg wrote:
> diff --git a/include/target/target_core_backend.h
> b/include/target/target_core_backend.h
> index 1e5c8f9..6ce370f 100644
> --- a/include/target/target_core_backend.h
> +++ b/include/target/target_core_backend.h
> @@ -3,6 +3,9 @@
> 
>  #define TRANSPORT_FLAG_PASSTHROUGH             1
> 
> +#define TARGET_SENSE_DESC_FORMAT(dev)          \
> +       dev->transport->get_blocks(dev) >= 0xffffffffULL

Make this a proper function, given that MODE SENSE isn't in the fast
path it doesn't even have to be inline.

Together with a fix for the comment from Bart this looks good to me.