[PATCH v1 0/4] Descriptor format sense data

[PATCH v1 0/4] 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 for LUs that spans 64bit sectors in order to avoid info
truncation (reported by hch).

Patch 4 is a nit, it was included in this to avoid dependencies
(patch 2/4)

Changes from v0:
- Added patch 4/4 to avoid dependencies
- Modified patch 3/4 to use descriptor format only for LUs that
  span 64bit sectors.
- Added reviwed-by tags.

Sagi Grimberg (4):
  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 in case the LU spans 64bit
    sectors
  libiscsi: Use scsi helper to set information descriptor

 drivers/ata/libata-scsi.c              |  4 +++-
 drivers/scsi/libiscsi.c                |  9 +++------
 drivers/scsi/scsi_common.c             | 15 +++++++++++++--
 drivers/target/target_core_hba.c       |  5 +++++
 drivers/target/target_core_spc.c       | 12 +++++++++---
 drivers/target/target_core_transport.c | 17 +++++++++++++----
 include/scsi/scsi_common.h             |  2 +-
 include/target/target_core_backend.h   |  2 ++
 8 files changed, 49 insertions(+), 17 deletions(-)

-- 
1.8.4.3

[PATCH v1 1/4] 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>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.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 v1 1/4] scsi: Fix wrong additional sense length in descriptor format

[Christoph Hellwig]

On Wed, Jul 15, 2015 at 10:55:36AM +0300, 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>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

Looks good,

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

[PATCH v1 2/4] 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 its callers scsi target and libata.

Reported-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.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 v1 2/4] scsi: Protect against buffer possible overflow in scsi_set_sense_information

[Christoph Hellwig]

On Wed, Jul 15, 2015 at 10:55:37AM +0300, Sagi Grimberg wrote:
> 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 its callers scsi target and libata.
> 
> Reported-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
> Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Tejun Heo <tj@kernel.org>

Looks good,

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

[PATCH v1 3/4] target: Return descriptor format sense data in case the LU spans 64bit sectors

[Sagi Grimberg]

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_hba.c       |  5 +++++
 drivers/target/target_core_spc.c       | 12 +++++++++---
 drivers/target/target_core_transport.c |  3 ++-
 include/target/target_core_backend.h   |  2 ++
 4 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_hba.c b/drivers/target/target_core_hba.c
index 62ea4e8..228e06d 100644
--- a/drivers/target/target_core_hba.c
+++ b/drivers/target/target_core_hba.c
@@ -176,3 +176,8 @@ core_delete_hba(struct se_hba *hba)
 	kfree(hba);
 	return 0;
 }
+
+bool target_sense_desc_format(struct se_device *dev)
+{
+	return dev->transport->get_blocks(dev) > 0xffffffffULL;
+}
diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index c43dcbf..b949d33 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..98155db 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..56cf8e4 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -93,4 +93,6 @@ bool	target_lun_is_rdonly(struct se_cmd *);
 sense_reason_t passthrough_parse_cdb(struct se_cmd *cmd,
 	sense_reason_t (*exec_cmd)(struct se_cmd *cmd));
 
+bool target_sense_desc_format(struct se_device *dev);
+
 #endif /* TARGET_CORE_BACKEND_H */
-- 
1.8.4.3

Re: [PATCH v1 3/4] target: Return descriptor format sense data in case the LU spans 64bit sectors

[Hannes Reinecke]

On 07/15/2015 09:55 AM, Sagi Grimberg wrote:
> 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_hba.c       |  5 +++++
>  drivers/target/target_core_spc.c       | 12 +++++++++---
>  drivers/target/target_core_transport.c |  3 ++-
>  include/target/target_core_backend.h   |  2 ++
>  4 files changed, 18 insertions(+), 4 deletions(-)
> 
We should eventually move to have the D_SENSE parameter changeable,
But in the meantime:

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)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/4] target: Return descriptor format sense data in case the LU spans 64bit sectors

[Christoph Hellwig]

On Wed, Jul 15, 2015 at 10:55:38AM +0300, Sagi Grimberg wrote:
> 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>

Looks good,

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

Re: [PATCH v1 3/4] target: Return descriptor format sense data in case the LU spans 64bit sectors

[Bart Van Assche]

On 07/15/15 00:55, Sagi Grimberg wrote:
> +bool target_sense_desc_format(struct se_device *dev)
> +{
> +	return dev->transport->get_blocks(dev) > 0xffffffffULL;
> +}

Please consider to use U32_MAX instead of 0xffffffffULL since that would 
make this function easier to read and to verify.

Bart.

Re: [PATCH v1 3/4] target: Return descriptor format sense data in case the LU spans 64bit sectors

[Martin K. Petersen]

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

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

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

I'm fine with Bart's suggestion.

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

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH v1 4/4] libiscsi: Use scsi helper to set information descriptor

[Sagi Grimberg]

In case encountered a PI error, use scsi_set_sense_information
instead of open coding information descriptor format.

Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
---
 drivers/scsi/libiscsi.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 8053f24..bb5ca7f 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -853,12 +853,9 @@ static void iscsi_scsi_cmd_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
 				     SAM_STAT_CHECK_CONDITION;
 			scsi_build_sense_buffer(1, sc->sense_buffer,
 						ILLEGAL_REQUEST, 0x10, ascq);
-			sc->sense_buffer[7] = 0xc; /* Additional sense length */
-			sc->sense_buffer[8] = 0;   /* Information desc type */
-			sc->sense_buffer[9] = 0xa; /* Additional desc length */
-			sc->sense_buffer[10] = 0x80; /* Validity bit */
-
-			put_unaligned_be64(sector, &sc->sense_buffer[12]);
+			scsi_set_sense_information(sc->sense_buffer,
+						   SCSI_SENSE_BUFFERSIZE,
+						   sector);
 			goto out;
 		}
 	}
-- 
1.8.4.3

Re: [PATCH v1 4/4] libiscsi: Use scsi helper to set information descriptor

[Hannes Reinecke]

On 07/15/2015 09:55 AM, Sagi Grimberg wrote:
> In case encountered a PI error, use scsi_set_sense_information
> instead of open coding information descriptor format.
> 
> Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
> ---
>  drivers/scsi/libiscsi.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
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 v1 4/4] libiscsi: Use scsi helper to set information descriptor

[Christoph Hellwig]

On Wed, Jul 15, 2015 at 10:55:39AM +0300, Sagi Grimberg wrote:
> In case encountered a PI error, use scsi_set_sense_information
> instead of open coding information descriptor format.
> 
> Signed-off-by: Sagi Grimberg <sagig@mellanox.com>

Looks good,

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

Re: [PATCH v1 4/4] libiscsi: Use scsi helper to set information descriptor

[Martin K. Petersen]

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

Sagi> In case encountered a PI error, use scsi_set_sense_information
Sagi> instead of open coding information descriptor format.

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

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v1 0/4] Descriptor format sense data

[Nicholas A. Bellinger]

On Wed, 2015-07-15 at 10:55 +0300, Sagi Grimberg wrote:
> 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 for LUs that spans 64bit sectors in order to avoid info
> truncation (reported by hch).
> 
> Patch 4 is a nit, it was included in this to avoid dependencies
> (patch 2/4)
> 
> Changes from v0:
> - Added patch 4/4 to avoid dependencies
> - Modified patch 3/4 to use descriptor format only for LUs that
>   span 64bit sectors.
> - Added reviwed-by tags.
> 
> Sagi Grimberg (4):
>   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 in case the LU spans 64bit
>     sectors
>   libiscsi: Use scsi helper to set information descriptor
> 
>  drivers/ata/libata-scsi.c              |  4 +++-
>  drivers/scsi/libiscsi.c                |  9 +++------
>  drivers/scsi/scsi_common.c             | 15 +++++++++++++--
>  drivers/target/target_core_hba.c       |  5 +++++
>  drivers/target/target_core_spc.c       | 12 +++++++++---
>  drivers/target/target_core_transport.c | 17 +++++++++++++----
>  include/scsi/scsi_common.h             |  2 +-
>  include/target/target_core_backend.h   |  2 ++
>  8 files changed, 49 insertions(+), 17 deletions(-)
> 

Applied to target-pending/for-next, with the updated patch #3 and
review-by tags.

Thanks folks.