[v2,07/10] qla2xxx: Terminate exchange if corrputed.

Message ID	1482357459-31079-8-git-send-email-himanshu.madhani@cavium.com (mailing list archive)
State	Accepted, archived
Headers	show Return-Path: <linux-scsi-owner@kernel.org> Received-SPF: None (protection.outlook.com: cavium.com does not designate permitted sender hosts) From: Himanshu Madhani <himanshu.madhani@cavium.com> To: <target-devel@vger.kernel.org>, <bart.vanassche@gmail.com>, <hch@infradead.org>, <nab@linux-iscsi.org> CC: <giridhar.malavali@cavium.com>, <linux-scsi@vger.kernel.org>, <himanshu.madhani@cavium.com> Subject: [PATCH v2 07/10] qla2xxx: Terminate exchange if corrputed. Date: Wed, 21 Dec 2016 13:57:36 -0800 Message-ID: <1482357459-31079-8-git-send-email-himanshu.madhani@cavium.com> In-Reply-To: <1482357459-31079-1-git-send-email-himanshu.madhani@cavium.com> References: <1482357459-31079-1-git-send-email-himanshu.madhani@cavium.com> MIME-Version: 1.0 Content-Type: text/plain SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1; SN2PR07MB030; 7:q+AeKAnat3tkq/SRDzo0/jeT8MB6ktqcW372FSV0MIL83QmZT5+dq5lQ7NxRFwmLdakEsDMOeYC9nyec+1vR5YgX0idfdelNeUEkvKkoQ3qlecz0l8X5rA0aYmeoQKFUe1RCziE7UAQuyGacd8yadskm+QgcS5qQVkN6xIMRDeuDCNsRUUv16OMRkLsyq/8PLbG2hSxfRtGoAivMarAwM2mCH95NM4zgaqDU0zWMEO5jiaLR9uyZg0hyRUyFzgMtoSa23vv7U6nYK2L9AXxTBxyhJWUuMJh9A8DqsbrIJGpMs4NQSRz+sjfq/xLMybTSttlz8qPNXD3kNgWst0w/eliiaSIfX1EyvRY7m34Fvk883ke9gqkbtd6IiFmFiVCjTNAwIrtzCrOAJh21B5/khEEmzh5NK/XwX8Jn8+DsPu0yN5fbKfgm41b2D0A0AftnlJpb3S73foiTgnSJb1m3CQ== Sender: linux-scsi-owner@vger.kernel.org Precedence: bulk

Message ID

1482357459-31079-8-git-send-email-himanshu.madhani@cavium.com (mailing list archive)

State

Accepted, archived

Headers

Received-SPF: None (protection.outlook.com: cavium.com does not designate
	permitted sender hosts)
From: Himanshu Madhani <himanshu.madhani@cavium.com>
To: <target-devel@vger.kernel.org>, <bart.vanassche@gmail.com>,
	<hch@infradead.org>, <nab@linux-iscsi.org>
CC: <giridhar.malavali@cavium.com>, <linux-scsi@vger.kernel.org>,
	<himanshu.madhani@cavium.com>
Subject: [PATCH v2 07/10] qla2xxx: Terminate exchange if corrputed.
Date: Wed, 21 Dec 2016 13:57:36 -0800
Message-ID: <1482357459-31079-8-git-send-email-himanshu.madhani@cavium.com>
In-Reply-To: <1482357459-31079-1-git-send-email-himanshu.madhani@cavium.com>
References: <1482357459-31079-1-git-send-email-himanshu.madhani@cavium.com>
MIME-Version: 1.0
Content-Type: text/plain
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Dec 2016 21:58:27.1733
	(UTC)
X-MS-Exchange-CrossTenant-Id: 711e4ccf-2e9b-4bcf-a551-4094005b6194
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=711e4ccf-2e9b-4bcf-a551-4094005b6194;
	Ip=[50.232.66.26]; Helo=[CAEXCH02.caveonetworks.com]
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN2PR07MB030
Sender: linux-scsi-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-scsi.vger.kernel.org>
X-Mailing-List: linux-scsi@vger.kernel.org
X-Virus-Scanned: ClamAV using ClamSMTP

Commit Message

Madhani, Himanshu Dec. 21, 2016, 9:57 p.m. UTC

From: Quinn Tran <quinn.tran@cavium.com>

Corrupted ATIO is defined as length of fcp_header & fcp_cmd
payload is less than 0x38. It's the minimum size for a frame to
carry 8..16 bytes SCSI CDB. The exchange will be dropped or
terminated if corrupted.

Signed-off-by: Quinn Tran <quinn.tran@cavium.com>
Signed-off-by: Himanshu Madhani <himanshu.madhani@cavium.com>
---
 drivers/scsi/qla2xxx/qla_def.h    |  3 ++-
 drivers/scsi/qla2xxx/qla_target.c | 22 +++++++++++++++++++---
 drivers/scsi/qla2xxx/qla_target.h | 22 +++++++++++++++++++++-
 3 files changed, 42 insertions(+), 5 deletions(-)

Comments

Bart Van Assche Dec. 23, 2016, 9:01 a.m. UTC | #1

On Wed, 2016-12-21 at 13:57 -0800, Himanshu Madhani wrote:
> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h

> index f7df01b..b14455e 100644

> --- a/drivers/scsi/qla2xxx/qla_def.h

> +++ b/drivers/scsi/qla2xxx/qla_def.h

> @@ -1556,7 +1556,8 @@ struct link_statistics {

>  struct atio {

>  	uint8_t		entry_type;		/* Entry type. */

>  	uint8_t		entry_count;		/* Entry count. */

> -	uint8_t		data[58];

> +	uint16_t	attr_n_length;

> +	uint8_t		data[56];

>  	uint32_t	signature;

>  #define ATIO_PROCESSED 0xDEADDEAD		/* Signature */

> };


Are struct atio and/or struct atio_from_isp the description of a firmware
data structure? If so, do all firmware versions initialize the attr_n_length
field or is there a minimum firmware version required for this field to be
valid? Please mention any changed dependencies on the firmware version in the
patch description. Please also fix the typo in the patch title when reposting
this patch ("corrputed").

> +			/* This packet is corrupted. The header + payload

> +			 * can not be trusted. There is no point in passing

> +			 * it further up.

> +			 */


This is not the proper Linux kernel comment style (see also
Documentation/process/coding-style.rst).

> diff --git a/drivers/scsi/qla2xxx/qla_target.h b/drivers/scsi/qla2xxx/qla_target.h

> index f26c5f6..f31d343 100644

> --- a/drivers/scsi/qla2xxx/qla_target.h

> +++ b/drivers/scsi/qla2xxx/qla_target.h

> @@ -427,13 +427,33 @@ struct atio_from_isp {

>  		struct {

>  			uint8_t  entry_type;	/* Entry type. */

>  			uint8_t  entry_count;	/* Entry count. */

> -			uint8_t  data[58];

> +			uint16_t attr_n_length;

> +#define FCP_CMD_LENGTH_MASK 0x0fff

> +#define FCP_CMD_LENGTH_MIN  0x38

> +			uint8_t  data[56];

>  			uint32_t signature;

>  #define ATIO_PROCESSED 0xDEADDEAD		/* Signature */

>  		} raw;

>  	} u;

>  } __packed;

>  

> +static inline int fcpcmd_is_corrupted(struct atio *atio)

> +{

> +	if (atio->entry_type == ATIO_TYPE7 &&

> +	    (le16_to_cpu(atio->attr_n_length & FCP_CMD_LENGTH_MASK) <

> +	    FCP_CMD_LENGTH_MIN))

> +		return 1;

> +	else

> +		return 0;

> +}


The above code shows that attr_n_length is a little endian field so please
declare it as little endian where appropriate (__le16 instead of uint16_t).

Bart.

Madhani, Himanshu Dec. 23, 2016, 11:55 p.m. UTC | #2

On 12/23/16, 1:01 AM, "Bart Van Assche" <Bart.VanAssche@sandisk.com> wrote:

>On Wed, 2016-12-21 at 13:57 -0800, Himanshu Madhani wrote:

>> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h

>> index f7df01b..b14455e 100644

>> --- a/drivers/scsi/qla2xxx/qla_def.h

>> +++ b/drivers/scsi/qla2xxx/qla_def.h

>> @@ -1556,7 +1556,8 @@ struct link_statistics {

>>  struct atio {

>>  	uint8_t		entry_type;		/* Entry type. */

>>  	uint8_t		entry_count;		/* Entry count. */

>> -	uint8_t		data[58];

>> +	uint16_t	attr_n_length;

>> +	uint8_t		data[56];

>>  	uint32_t	signature;

>>  #define ATIO_PROCESSED 0xDEADDEAD		/* Signature */

>> };

>

>Are struct atio and/or struct atio_from_isp the description of a firmware

>data structure? If so, do all firmware versions initialize the attr_n_length

>field or is there a minimum firmware version required for this field to be

>valid? Please mention any changed dependencies on the firmware version in the

>patch description. Please also fix the typo in the patch title when reposting

>this patch ("corrputed").


They represent same data structure from firmware. We will send follow up patch to
Clean up redundant structure.

>

>> +			/* This packet is corrupted. The header + payload

>> +			 * can not be trusted. There is no point in passing

>> +			 * it further up.

>> +			 */

>

>This is not the proper Linux kernel comment style (see also

>Documentation/process/coding-style.rst).


Ack. Will update patch with correct format.

>

>> diff --git a/drivers/scsi/qla2xxx/qla_target.h b/drivers/scsi/qla2xxx/qla_target.h

>> index f26c5f6..f31d343 100644

>> --- a/drivers/scsi/qla2xxx/qla_target.h

>> +++ b/drivers/scsi/qla2xxx/qla_target.h

>> @@ -427,13 +427,33 @@ struct atio_from_isp {

>>  		struct {

>>  			uint8_t  entry_type;	/* Entry type. */

>>  			uint8_t  entry_count;	/* Entry count. */

>> -			uint8_t  data[58];

>> +			uint16_t attr_n_length;

>> +#define FCP_CMD_LENGTH_MASK 0x0fff

>> +#define FCP_CMD_LENGTH_MIN  0x38

>> +			uint8_t  data[56];

>>  			uint32_t signature;

>>  #define ATIO_PROCESSED 0xDEADDEAD		/* Signature */

>>  		} raw;

>>  	} u;

>>  } __packed;

>>  

>> +static inline int fcpcmd_is_corrupted(struct atio *atio)

>> +{

>> +	if (atio->entry_type == ATIO_TYPE7 &&

>> +	    (le16_to_cpu(atio->attr_n_length & FCP_CMD_LENGTH_MASK) <

>> +	    FCP_CMD_LENGTH_MIN))

>> +		return 1;

>> +	else

>> +		return 0;

>> +}

>

>The above code shows that attr_n_length is a little endian field so please

>declare it as little endian where appropriate (__le16 instead of uint16_t).


Ack. Will update the patch

>

>Bart.

diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
index f7df01b..b14455e 100644
--- a/drivers/scsi/qla2xxx/qla_def.h
+++ b/drivers/scsi/qla2xxx/qla_def.h
@@ -1556,7 +1556,8 @@  struct link_statistics {
 struct atio {
 	uint8_t		entry_type;		/* Entry type. */
 	uint8_t		entry_count;		/* Entry count. */
-	uint8_t		data[58];
+	uint16_t	attr_n_length;
+	uint8_t		data[56];
 	uint32_t	signature;
 #define ATIO_PROCESSED 0xDEADDEAD		/* Signature */
 };
diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index 5037b51..9252694 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -6451,12 +6451,28 @@  static void qlt_disable_vha(struct scsi_qla_host *vha)
 	if (!vha->flags.online)
 		return;
 
-	while (ha->tgt.atio_ring_ptr->signature != ATIO_PROCESSED) {
+	while ((ha->tgt.atio_ring_ptr->signature != ATIO_PROCESSED) ||
+	    fcpcmd_is_corrupted(ha->tgt.atio_ring_ptr)) {
 		pkt = (struct atio_from_isp *)ha->tgt.atio_ring_ptr;
 		cnt = pkt->u.raw.entry_count;
 
-		qlt_24xx_atio_pkt_all_vps(vha, (struct atio_from_isp *)pkt,
-		    ha_locked);
+		if (unlikely(fcpcmd_is_corrupted(ha->tgt.atio_ring_ptr))) {
+			/* This packet is corrupted. The header + payload
+			 * can not be trusted. There is no point in passing
+			 * it further up.
+			 */
+			ql_log(ql_log_warn, vha, 0xffff,
+			    "corrupted fcp frame SID[%3phN] OXID[%04x] EXCG[%x] %64phN\n",
+			    pkt->u.isp24.fcp_hdr.s_id,
+			    be16_to_cpu(pkt->u.isp24.fcp_hdr.ox_id),
+			    le32_to_cpu(pkt->u.isp24.exchange_addr), pkt);
+
+			adjust_corrupted_atio(pkt);
+			qlt_send_term_exchange(vha, NULL, pkt, ha_locked, 0);
+		} else {
+			qlt_24xx_atio_pkt_all_vps(vha,
+			    (struct atio_from_isp *)pkt, ha_locked);
+		}
 
 		for (i = 0; i < cnt; i++) {
 			ha->tgt.atio_ring_index++;
diff --git a/drivers/scsi/qla2xxx/qla_target.h b/drivers/scsi/qla2xxx/qla_target.h
index f26c5f6..f31d343 100644
--- a/drivers/scsi/qla2xxx/qla_target.h
+++ b/drivers/scsi/qla2xxx/qla_target.h
@@ -427,13 +427,33 @@  struct atio_from_isp {
 		struct {
 			uint8_t  entry_type;	/* Entry type. */
 			uint8_t  entry_count;	/* Entry count. */
-			uint8_t  data[58];
+			uint16_t attr_n_length;
+#define FCP_CMD_LENGTH_MASK 0x0fff
+#define FCP_CMD_LENGTH_MIN  0x38
+			uint8_t  data[56];
 			uint32_t signature;
 #define ATIO_PROCESSED 0xDEADDEAD		/* Signature */
 		} raw;
 	} u;
 } __packed;
 
+static inline int fcpcmd_is_corrupted(struct atio *atio)
+{
+	if (atio->entry_type == ATIO_TYPE7 &&
+	    (le16_to_cpu(atio->attr_n_length & FCP_CMD_LENGTH_MASK) <
+	    FCP_CMD_LENGTH_MIN))
+		return 1;
+	else
+		return 0;
+}
+
+/* adjust corrupted atio so we won't trip over the same entry again. */
+static inline void adjust_corrupted_atio(struct atio_from_isp *atio)
+{
+	atio->u.raw.attr_n_length = cpu_to_le16(FCP_CMD_LENGTH_MIN);
+	atio->u.isp24.fcp_cmnd.add_cdb_len = 0;
+}
+
 #define CTIO_TYPE7 0x12 /* Continue target I/O entry (for 24xx) */
 
 /*

[v2,07/10] qla2xxx: Terminate exchange if corrputed.

Commit Message

Comments

Patch