All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/8] qla2xxx klocwork fixes
@ 2023-05-18  7:58 Nilesh Javali
  2023-05-18  7:58 ` [PATCH 1/8] qla2xxx: klocwork - Array index may go out of bound Nilesh Javali
                   ` (7 more replies)
  0 siblings, 8 replies; 15+ messages in thread
From: Nilesh Javali @ 2023-05-18  7:58 UTC (permalink / raw)
  To: martin.petersen
  Cc: linux-scsi, GR-QLogic-Storage-Upstream, bhazarika, agurumurthy, sdeodhar

Martin,

Please apply the qla2xxx driver klocwork fixes to
the scsi tree at your earliest convenience.

Thanks,
Nilesh

Bikash Hazarika (2):
  qla2xxx: klocwork - Fix potential null pointer dereference
  qla2xxx: klocwork - correct the index of array

Nilesh Javali (4):
  qla2xxx: klocwork - Array index may go out of bound
  qla2xxx: klocwork - Check for a valid fcport pointer
  qla2xxx: klocwork - Check valid rport returned by fc_bsg_to_rport
  qla2xxx: Update version to 10.02.08.400-k

Quinn Tran (1):
  qla2xxx: klocwork - Fix buffer overrun

Shreyas Deodhar (1):
  qla2xxx: klocwork - pointer may be dereferenced

 drivers/scsi/qla2xxx/qla_bsg.c     | 6 ++++++
 drivers/scsi/qla2xxx/qla_edif.c    | 3 ++-
 drivers/scsi/qla2xxx/qla_init.c    | 2 +-
 drivers/scsi/qla2xxx/qla_inline.h  | 5 ++++-
 drivers/scsi/qla2xxx/qla_iocb.c    | 8 +++++---
 drivers/scsi/qla2xxx/qla_os.c      | 3 ++-
 drivers/scsi/qla2xxx/qla_version.h | 4 ++--
 7 files changed, 22 insertions(+), 9 deletions(-)


base-commit: 44ef1604ae9492a7d9238ea79aa0cc7b4c4de860
-- 
2.23.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/8] qla2xxx: klocwork - Array index may go out of bound
  2023-05-18  7:58 [PATCH 0/8] qla2xxx klocwork fixes Nilesh Javali
@ 2023-05-18  7:58 ` Nilesh Javali
  2023-05-18  7:58 ` [PATCH 2/8] qla2xxx: klocwork - Fix potential null pointer dereference Nilesh Javali
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 15+ messages in thread
From: Nilesh Javali @ 2023-05-18  7:58 UTC (permalink / raw)
  To: martin.petersen
  Cc: linux-scsi, GR-QLogic-Storage-Upstream, bhazarika, agurumurthy, sdeodhar

Klocwork reports array 'vha->host_str' of size 16
may use index value(s) 16..19.
Use snprintf instead of sprintf.

Cc: stable@vger.kernel.org
Signed-off-by: Bikash Hazarika <bhazarika@marvell.com>
Signed-off-by: Nilesh Javali <njavali@marvell.com>
---
 drivers/scsi/qla2xxx/qla_os.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index bc89d3da8fd0..3bace9ea6288 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -5088,7 +5088,8 @@ struct scsi_qla_host *qla2x00_create_host(const struct scsi_host_template *sht,
 	}
 	INIT_DELAYED_WORK(&vha->scan.scan_work, qla_scan_work_fn);
 
-	sprintf(vha->host_str, "%s_%lu", QLA2XXX_DRIVER_NAME, vha->host_no);
+	snprintf(vha->host_str, sizeof(vha->host_str), "%s_%lu",
+		 QLA2XXX_DRIVER_NAME, vha->host_no);
 	ql_dbg(ql_dbg_init, vha, 0x0041,
 	    "Allocated the host=%p hw=%p vha=%p dev_name=%s",
 	    vha->host, vha->hw, vha,
-- 
2.23.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/8] qla2xxx: klocwork - Fix potential null pointer dereference
  2023-05-18  7:58 [PATCH 0/8] qla2xxx klocwork fixes Nilesh Javali
  2023-05-18  7:58 ` [PATCH 1/8] qla2xxx: klocwork - Array index may go out of bound Nilesh Javali
@ 2023-05-18  7:58 ` Nilesh Javali
  2023-05-18 17:42   ` Bart Van Assche
  2023-05-18  7:58 ` [PATCH 3/8] qla2xxx: klocwork - Check for a valid fcport pointer Nilesh Javali
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 15+ messages in thread
From: Nilesh Javali @ 2023-05-18  7:58 UTC (permalink / raw)
  To: martin.petersen
  Cc: linux-scsi, GR-QLogic-Storage-Upstream, bhazarika, agurumurthy, sdeodhar

From: Bikash Hazarika <bhazarika@marvell.com>

Klocwork tool reported 'cur_dsd' may be dereferenced.
Add fix to validate pointer before dereferencing
the pointer.

Cc: stable@vger.kernel.org
Signed-off-by: Bikash Hazarika <bhazarika@marvell.com>
Signed-off-by: Nilesh Javali <njavali@marvell.com>
---
 drivers/scsi/qla2xxx/qla_iocb.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
index 6acfdcc48b16..a092151aef77 100644
--- a/drivers/scsi/qla2xxx/qla_iocb.c
+++ b/drivers/scsi/qla2xxx/qla_iocb.c
@@ -664,9 +664,11 @@ qla24xx_build_scsi_type_6_iocbs(srb_t *sp, struct cmd_type_6 *cmd_pkt,
 	}
 
 	/* Null termination */
-	cur_dsd->address = 0;
-	cur_dsd->length = 0;
-	cur_dsd++;
+	if (cur_dsd) {
+		cur_dsd->address = 0;
+		cur_dsd->length = 0;
+		cur_dsd++;
+	}
 	cmd_pkt->control_flags |= cpu_to_le16(CF_DATA_SEG_DESCR_ENABLE);
 	return 0;
 }
-- 
2.23.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/8] qla2xxx: klocwork - Check for a valid fcport pointer
  2023-05-18  7:58 [PATCH 0/8] qla2xxx klocwork fixes Nilesh Javali
  2023-05-18  7:58 ` [PATCH 1/8] qla2xxx: klocwork - Array index may go out of bound Nilesh Javali
  2023-05-18  7:58 ` [PATCH 2/8] qla2xxx: klocwork - Fix potential null pointer dereference Nilesh Javali
@ 2023-05-18  7:58 ` Nilesh Javali
  2023-05-18 17:44   ` Bart Van Assche
  2023-05-18  7:58 ` [PATCH 4/8] qla2xxx: klocwork - Check valid rport returned by fc_bsg_to_rport Nilesh Javali
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 15+ messages in thread
From: Nilesh Javali @ 2023-05-18  7:58 UTC (permalink / raw)
  To: martin.petersen
  Cc: linux-scsi, GR-QLogic-Storage-Upstream, bhazarika, agurumurthy, sdeodhar

Klocwork reported warning of null pointer may be dereferenced.
The routine exits when sa_ctl is NULL and fcport is allocated after
the exit call thus causing NULL fcport pointer to dereference at the
time of exit.

Add a check for a valid fcport pointer at the time of exit.

Cc: stable@vger.kernel.org
Signed-off-by: Nilesh Javali <njavali@marvell.com>
---
 drivers/scsi/qla2xxx/qla_edif.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/qla_edif.c b/drivers/scsi/qla2xxx/qla_edif.c
index ec0e20255bd3..14e314c12dd6 100644
--- a/drivers/scsi/qla2xxx/qla_edif.c
+++ b/drivers/scsi/qla2xxx/qla_edif.c
@@ -2411,7 +2411,8 @@ qla24xx_issue_sa_replace_iocb(scsi_qla_host_t *vha, struct qla_work_evt *e)
 	kref_put(&sp->cmd_kref, qla2x00_sp_release);
 	fcport->flags &= ~FCF_ASYNC_SENT;
 done:
-	fcport->flags &= ~FCF_ASYNC_ACTIVE;
+	if (fcport)
+		fcport->flags &= ~FCF_ASYNC_ACTIVE;
 	return rval;
 }
 
-- 
2.23.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 4/8] qla2xxx: klocwork - Check valid rport returned by fc_bsg_to_rport
  2023-05-18  7:58 [PATCH 0/8] qla2xxx klocwork fixes Nilesh Javali
                   ` (2 preceding siblings ...)
  2023-05-18  7:58 ` [PATCH 3/8] qla2xxx: klocwork - Check for a valid fcport pointer Nilesh Javali
@ 2023-05-18  7:58 ` Nilesh Javali
  2023-05-18  7:58 ` [PATCH 5/8] qla2xxx: klocwork - Fix buffer overrun Nilesh Javali
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 15+ messages in thread
From: Nilesh Javali @ 2023-05-18  7:58 UTC (permalink / raw)
  To: martin.petersen
  Cc: linux-scsi, GR-QLogic-Storage-Upstream, bhazarika, agurumurthy, sdeodhar

Klocwork reported warning of rport maybe NULL
and will be dereferenced.
rport returned by call to fc_bsg_to_rport could
be NULL and dereferenced.

Check valid rport returned by fc_bsg_to_rport.

Cc: stable@vger.kernel.org
Signed-off-by: Nilesh Javali <njavali@marvell.com>
---
 drivers/scsi/qla2xxx/qla_bsg.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
index dba7bba788d7..c928b27061a9 100644
--- a/drivers/scsi/qla2xxx/qla_bsg.c
+++ b/drivers/scsi/qla2xxx/qla_bsg.c
@@ -283,6 +283,10 @@ qla2x00_process_els(struct bsg_job *bsg_job)
 
 	if (bsg_request->msgcode == FC_BSG_RPT_ELS) {
 		rport = fc_bsg_to_rport(bsg_job);
+		if (!rport) {
+			rval = -ENOMEM;
+			goto done;
+		}
 		fcport = *(fc_port_t **) rport->dd_data;
 		host = rport_to_shost(rport);
 		vha = shost_priv(host);
-- 
2.23.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 5/8] qla2xxx: klocwork - Fix buffer overrun
  2023-05-18  7:58 [PATCH 0/8] qla2xxx klocwork fixes Nilesh Javali
                   ` (3 preceding siblings ...)
  2023-05-18  7:58 ` [PATCH 4/8] qla2xxx: klocwork - Check valid rport returned by fc_bsg_to_rport Nilesh Javali
@ 2023-05-18  7:58 ` Nilesh Javali
  2023-05-18  7:58 ` [PATCH 6/8] qla2xxx: klocwork - pointer may be dereferenced Nilesh Javali
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 15+ messages in thread
From: Nilesh Javali @ 2023-05-18  7:58 UTC (permalink / raw)
  To: martin.petersen
  Cc: linux-scsi, GR-QLogic-Storage-Upstream, bhazarika, agurumurthy, sdeodhar

From: Quinn Tran <qutran@marvell.com>

Klocwork warning: Buffer Overflow - Array Index Out of Bounds

Driver uses fc_els_flogi to calculate size of buffer.
The actual buffer is nested inside of fc_els_flogi
which is smaller.

Replace structure name to allow proper size calculation.

Cc: stable@vger.kernel.org
Signed-off-by: Quinn Tran <qutran@marvell.com>
Signed-off-by: Nilesh Javali <njavali@marvell.com>
---
 drivers/scsi/qla2xxx/qla_init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
index 0df6eae7324e..b0225f6f3221 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -5549,7 +5549,7 @@ static void qla_get_login_template(scsi_qla_host_t *vha)
 	__be32 *q;
 
 	memset(ha->init_cb, 0, ha->init_cb_size);
-	sz = min_t(int, sizeof(struct fc_els_flogi), ha->init_cb_size);
+	sz = min_t(int, sizeof(struct fc_els_csp), ha->init_cb_size);
 	rval = qla24xx_get_port_login_templ(vha, ha->init_cb_dma,
 					    ha->init_cb, sz);
 	if (rval != QLA_SUCCESS) {
-- 
2.23.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 6/8] qla2xxx: klocwork - pointer may be dereferenced
  2023-05-18  7:58 [PATCH 0/8] qla2xxx klocwork fixes Nilesh Javali
                   ` (4 preceding siblings ...)
  2023-05-18  7:58 ` [PATCH 5/8] qla2xxx: klocwork - Fix buffer overrun Nilesh Javali
@ 2023-05-18  7:58 ` Nilesh Javali
  2023-05-18  7:58 ` [PATCH 7/8] qla2xxx: klocwork - correct the index of array Nilesh Javali
  2023-05-18  7:58 ` [PATCH 8/8] qla2xxx: Update version to 10.02.08.400-k Nilesh Javali
  7 siblings, 0 replies; 15+ messages in thread
From: Nilesh Javali @ 2023-05-18  7:58 UTC (permalink / raw)
  To: martin.petersen
  Cc: linux-scsi, GR-QLogic-Storage-Upstream, bhazarika, agurumurthy, sdeodhar

From: Shreyas Deodhar <sdeodhar@marvell.com>

Klocwork tool reported pointer 'rport' returned
from call to function 'fc_bsg_to_rport' may be
NULL and will be dereferenced.

Add a fix to validate rport before dereferencing.

Cc: stable@vger.kernel.org
Signed-off-by: Shreyas Deodhar <sdeodhar@marvell.com>
Signed-off-by: Nilesh Javali <njavali@marvell.com>
---
 drivers/scsi/qla2xxx/qla_bsg.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
index c928b27061a9..19bb64bdd88b 100644
--- a/drivers/scsi/qla2xxx/qla_bsg.c
+++ b/drivers/scsi/qla2xxx/qla_bsg.c
@@ -2996,6 +2996,8 @@ qla24xx_bsg_request(struct bsg_job *bsg_job)
 
 	if (bsg_request->msgcode == FC_BSG_RPT_ELS) {
 		rport = fc_bsg_to_rport(bsg_job);
+		if (!rport)
+			return ret;
 		host = rport_to_shost(rport);
 		vha = shost_priv(host);
 	} else {
-- 
2.23.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 7/8] qla2xxx: klocwork - correct the index of array
  2023-05-18  7:58 [PATCH 0/8] qla2xxx klocwork fixes Nilesh Javali
                   ` (5 preceding siblings ...)
  2023-05-18  7:58 ` [PATCH 6/8] qla2xxx: klocwork - pointer may be dereferenced Nilesh Javali
@ 2023-05-18  7:58 ` Nilesh Javali
  2023-05-18 17:47   ` Bart Van Assche
  2023-05-18  7:58 ` [PATCH 8/8] qla2xxx: Update version to 10.02.08.400-k Nilesh Javali
  7 siblings, 1 reply; 15+ messages in thread
From: Nilesh Javali @ 2023-05-18  7:58 UTC (permalink / raw)
  To: martin.petersen
  Cc: linux-scsi, GR-QLogic-Storage-Upstream, bhazarika, agurumurthy, sdeodhar

From: Bikash Hazarika <bhazarika@marvell.com>

Klocwork reported array 'port_dstate_str' of size
10 may use index value(s) 10..15.

Add a fix to correct the index of array.

Cc: stable@vger.kernel.org
Signed-off-by: Bikash Hazarika <bhazarika@marvell.com>
Signed-off-by: Nilesh Javali <njavali@marvell.com>
---
 drivers/scsi/qla2xxx/qla_inline.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/qla_inline.h b/drivers/scsi/qla2xxx/qla_inline.h
index cce6e425c121..ee70cf337242 100644
--- a/drivers/scsi/qla2xxx/qla_inline.h
+++ b/drivers/scsi/qla2xxx/qla_inline.h
@@ -109,11 +109,13 @@ qla2x00_set_fcport_disc_state(fc_port_t *fcport, int state)
 {
 	int old_val;
 	uint8_t shiftbits, mask;
+	uint8_t port_dstate_str_sz;
 
 	/* This will have to change when the max no. of states > 16 */
 	shiftbits = 4;
 	mask = (1 << shiftbits) - 1;
 
+	port_dstate_str_sz = sizeof(port_dstate_str)/sizeof(char *);
 	fcport->disc_state = state;
 	while (1) {
 		old_val = atomic_read(&fcport->shadow_disc_state);
@@ -121,7 +123,8 @@ qla2x00_set_fcport_disc_state(fc_port_t *fcport, int state)
 		    old_val, (old_val << shiftbits) | state)) {
 			ql_dbg(ql_dbg_disc, fcport->vha, 0x2134,
 			    "FCPort %8phC disc_state transition: %s to %s - portid=%06x.\n",
-			    fcport->port_name, port_dstate_str[old_val & mask],
+			    fcport->port_name, ((old_val & mask) < port_dstate_str_sz) ?
+				    port_dstate_str[old_val & mask] : "Unknown",
 			    port_dstate_str[state], fcport->d_id.b24);
 			return;
 		}
-- 
2.23.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 8/8] qla2xxx: Update version to 10.02.08.400-k
  2023-05-18  7:58 [PATCH 0/8] qla2xxx klocwork fixes Nilesh Javali
                   ` (6 preceding siblings ...)
  2023-05-18  7:58 ` [PATCH 7/8] qla2xxx: klocwork - correct the index of array Nilesh Javali
@ 2023-05-18  7:58 ` Nilesh Javali
  7 siblings, 0 replies; 15+ messages in thread
From: Nilesh Javali @ 2023-05-18  7:58 UTC (permalink / raw)
  To: martin.petersen
  Cc: linux-scsi, GR-QLogic-Storage-Upstream, bhazarika, agurumurthy, sdeodhar

Signed-off-by: Nilesh Javali <njavali@marvell.com>
---
 drivers/scsi/qla2xxx/qla_version.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_version.h b/drivers/scsi/qla2xxx/qla_version.h
index 4d6f06fb156b..e3771923b0d7 100644
--- a/drivers/scsi/qla2xxx/qla_version.h
+++ b/drivers/scsi/qla2xxx/qla_version.h
@@ -6,9 +6,9 @@
 /*
  * Driver version
  */
-#define QLA2XXX_VERSION      "10.02.08.300-k"
+#define QLA2XXX_VERSION      "10.02.08.400-k"
 
 #define QLA_DRIVER_MAJOR_VER	10
 #define QLA_DRIVER_MINOR_VER	2
 #define QLA_DRIVER_PATCH_VER	8
-#define QLA_DRIVER_BETA_VER	300
+#define QLA_DRIVER_BETA_VER	400
-- 
2.23.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/8] qla2xxx: klocwork - Fix potential null pointer dereference
  2023-05-18  7:58 ` [PATCH 2/8] qla2xxx: klocwork - Fix potential null pointer dereference Nilesh Javali
@ 2023-05-18 17:42   ` Bart Van Assche
  2023-05-31 11:43     ` [EXT] " Nilesh Javali
  0 siblings, 1 reply; 15+ messages in thread
From: Bart Van Assche @ 2023-05-18 17:42 UTC (permalink / raw)
  To: Nilesh Javali, martin.petersen
  Cc: linux-scsi, GR-QLogic-Storage-Upstream, bhazarika, agurumurthy, sdeodhar

On 5/18/23 00:58, Nilesh Javali wrote:
> From: Bikash Hazarika <bhazarika@marvell.com>
> 
> Klocwork tool reported 'cur_dsd' may be dereferenced.
> Add fix to validate pointer before dereferencing
> the pointer.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Bikash Hazarika <bhazarika@marvell.com>
> Signed-off-by: Nilesh Javali <njavali@marvell.com>
> ---
>   drivers/scsi/qla2xxx/qla_iocb.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
> index 6acfdcc48b16..a092151aef77 100644
> --- a/drivers/scsi/qla2xxx/qla_iocb.c
> +++ b/drivers/scsi/qla2xxx/qla_iocb.c
> @@ -664,9 +664,11 @@ qla24xx_build_scsi_type_6_iocbs(srb_t *sp, struct cmd_type_6 *cmd_pkt,
>   	}
>   
>   	/* Null termination */
> -	cur_dsd->address = 0;
> -	cur_dsd->length = 0;
> -	cur_dsd++;
> +	if (cur_dsd) {
> +		cur_dsd->address = 0;
> +		cur_dsd->length = 0;
> +		cur_dsd++;
> +	}
>   	cmd_pkt->control_flags |= cpu_to_le16(CF_DATA_SEG_DESCR_ENABLE);
>   	return 0;
>   }

Please add BUG_ON(!cur_dsd) above the first cur_dsd dereference instead 
of making the above change. The above change hides a bug. Hiding bugs 
doesn't help anyone.

Bart.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/8] qla2xxx: klocwork - Check for a valid fcport pointer
  2023-05-18  7:58 ` [PATCH 3/8] qla2xxx: klocwork - Check for a valid fcport pointer Nilesh Javali
@ 2023-05-18 17:44   ` Bart Van Assche
  0 siblings, 0 replies; 15+ messages in thread
From: Bart Van Assche @ 2023-05-18 17:44 UTC (permalink / raw)
  To: Nilesh Javali, martin.petersen
  Cc: linux-scsi, GR-QLogic-Storage-Upstream, bhazarika, agurumurthy, sdeodhar

On 5/18/23 00:58, Nilesh Javali wrote:
> Klocwork reported warning of null pointer may be dereferenced.
> The routine exits when sa_ctl is NULL and fcport is allocated after
> the exit call thus causing NULL fcport pointer to dereference at the
> time of exit.
> 
> Add a check for a valid fcport pointer at the time of exit.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Nilesh Javali <njavali@marvell.com>
> ---
>   drivers/scsi/qla2xxx/qla_edif.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_edif.c b/drivers/scsi/qla2xxx/qla_edif.c
> index ec0e20255bd3..14e314c12dd6 100644
> --- a/drivers/scsi/qla2xxx/qla_edif.c
> +++ b/drivers/scsi/qla2xxx/qla_edif.c
> @@ -2411,7 +2411,8 @@ qla24xx_issue_sa_replace_iocb(scsi_qla_host_t *vha, struct qla_work_evt *e)
>   	kref_put(&sp->cmd_kref, qla2x00_sp_release);
>   	fcport->flags &= ~FCF_ASYNC_SENT;
>   done:
> -	fcport->flags &= ~FCF_ASYNC_ACTIVE;
> +	if (fcport)
> +		fcport->flags &= ~FCF_ASYNC_ACTIVE;
>   	return rval;
>   }

Please change the "goto done" statements that occur before fcport is set 
into "return rval" instead of making the above change.

Bart.



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 7/8] qla2xxx: klocwork - correct the index of array
  2023-05-18  7:58 ` [PATCH 7/8] qla2xxx: klocwork - correct the index of array Nilesh Javali
@ 2023-05-18 17:47   ` Bart Van Assche
  0 siblings, 0 replies; 15+ messages in thread
From: Bart Van Assche @ 2023-05-18 17:47 UTC (permalink / raw)
  To: Nilesh Javali, martin.petersen
  Cc: linux-scsi, GR-QLogic-Storage-Upstream, bhazarika, agurumurthy, sdeodhar

On 5/18/23 00:58, Nilesh Javali wrote:
> +	port_dstate_str_sz = sizeof(port_dstate_str)/sizeof(char *);

Please use ARRAY_SIZE() instead of open-coding it.

> @@ -121,7 +123,8 @@ qla2x00_set_fcport_disc_state(fc_port_t *fcport, int state)
>   		    old_val, (old_val << shiftbits) | state)) {
>   			ql_dbg(ql_dbg_disc, fcport->vha, 0x2134,
>   			    "FCPort %8phC disc_state transition: %s to %s - portid=%06x.\n",
> -			    fcport->port_name, port_dstate_str[old_val & mask],
> +			    fcport->port_name, ((old_val & mask) < port_dstate_str_sz) ?
> +				    port_dstate_str[old_val & mask] : "Unknown",
>   			    port_dstate_str[state], fcport->d_id.b24);

Please do not introduce more parentheses than necessary. The outer 
parentheses can be removed from the ((old_val & mask) < 
port_dstate_str_sz) expression without reducing readability.

Thanks,

Bart.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [EXT] Re: [PATCH 2/8] qla2xxx: klocwork - Fix potential null pointer dereference
  2023-05-18 17:42   ` Bart Van Assche
@ 2023-05-31 11:43     ` Nilesh Javali
  2023-05-31 12:33       ` Bart Van Assche
  0 siblings, 1 reply; 15+ messages in thread
From: Nilesh Javali @ 2023-05-31 11:43 UTC (permalink / raw)
  To: Bart Van Assche, martin.petersen
  Cc: linux-scsi, GR-QLogic-Storage-Upstream, Bikash Hazarika,
	Anil Gurumurthy, Shreyas Deodhar


> -----Original Message-----
> From: Bart Van Assche <bvanassche@acm.org>
> Sent: Thursday, May 18, 2023 11:12 PM
> To: Nilesh Javali <njavali@marvell.com>; martin.petersen@oracle.com
> Cc: linux-scsi@vger.kernel.org; GR-QLogic-Storage-Upstream <GR-QLogic-
> Storage-Upstream@marvell.com>; Bikash Hazarika <bhazarika@marvell.com>;
> Anil Gurumurthy <agurumurthy@marvell.com>; Shreyas Deodhar
> <sdeodhar@marvell.com>
> Subject: [EXT] Re: [PATCH 2/8] qla2xxx: klocwork - Fix potential null pointer
> dereference
> 
> External Email
> 
> ----------------------------------------------------------------------
> On 5/18/23 00:58, Nilesh Javali wrote:
> > From: Bikash Hazarika <bhazarika@marvell.com>
> >
> > Klocwork tool reported 'cur_dsd' may be dereferenced.
> > Add fix to validate pointer before dereferencing
> > the pointer.
> >
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Bikash Hazarika <bhazarika@marvell.com>
> > Signed-off-by: Nilesh Javali <njavali@marvell.com>
> > ---
> >   drivers/scsi/qla2xxx/qla_iocb.c | 8 +++++---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
> > index 6acfdcc48b16..a092151aef77 100644
> > --- a/drivers/scsi/qla2xxx/qla_iocb.c
> > +++ b/drivers/scsi/qla2xxx/qla_iocb.c
> > @@ -664,9 +664,11 @@ qla24xx_build_scsi_type_6_iocbs(srb_t *sp, struct
> cmd_type_6 *cmd_pkt,
> >   	}
> >
> >   	/* Null termination */
> > -	cur_dsd->address = 0;
> > -	cur_dsd->length = 0;
> > -	cur_dsd++;
> > +	if (cur_dsd) {
> > +		cur_dsd->address = 0;
> > +		cur_dsd->length = 0;
> > +		cur_dsd++;
> > +	}
> >   	cmd_pkt->control_flags |=
> cpu_to_le16(CF_DATA_SEG_DESCR_ENABLE);
> >   	return 0;
> >   }
> 
> Please add BUG_ON(!cur_dsd) above the first cur_dsd dereference instead
> of making the above change. The above change hides a bug. Hiding bugs
> doesn't help anyone.
> 
> Bart.

Thanks for the review.
We can prevent the crash and notify the occurrence of this
rare case by adding warn_on like,

+       WARN_ON_ONCE(!cur_dsd);
+       if (cur_dsd) {
+               cur_dsd->address = 0;
+               cur_dsd->length = 0;
+               cur_dsd++;
+       }
        cmd_pkt->control_flags |= cpu_to_le16(CF_DATA_SEG_DESCR_ENABLE);
        return 0;
 }

Thanks,
Nilesh

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [EXT] Re: [PATCH 2/8] qla2xxx: klocwork - Fix potential null pointer dereference
  2023-05-31 11:43     ` [EXT] " Nilesh Javali
@ 2023-05-31 12:33       ` Bart Van Assche
  2023-06-07  8:31         ` Nilesh Javali
  0 siblings, 1 reply; 15+ messages in thread
From: Bart Van Assche @ 2023-05-31 12:33 UTC (permalink / raw)
  To: Nilesh Javali, martin.petersen
  Cc: linux-scsi, GR-QLogic-Storage-Upstream, Bikash Hazarika,
	Anil Gurumurthy, Shreyas Deodhar

On 5/31/23 04:43, Nilesh Javali wrote:
> We can prevent the crash and notify the occurrence of this
> rare case by adding warn_on like,
> 
> +       WARN_ON_ONCE(!cur_dsd);
> +       if (cur_dsd) {
> +               cur_dsd->address = 0;
> +               cur_dsd->length = 0;
> +               cur_dsd++;
> +       }
>          cmd_pkt->control_flags |= cpu_to_le16(CF_DATA_SEG_DESCR_ENABLE);
>          return 0;
>   }

I think there is a much better solution: drop the new "if (cur_dsd) {" 
test and instead add the following code:

diff --git a/drivers/scsi/qla2xxx/qla_iocb.c 
b/drivers/scsi/qla2xxx/qla_iocb.c
index 6acfdcc48b16..a1675f056a5c 100644
--- a/drivers/scsi/qla2xxx/qla_iocb.c
+++ b/drivers/scsi/qla2xxx/qla_iocb.c
@@ -607,7 +607,8 @@ qla24xx_build_scsi_type_6_iocbs(srb_t *sp, struct 
cmd_type_6 *cmd_pkt,
  	put_unaligned_le32(COMMAND_TYPE_6, &cmd_pkt->entry_type);

  	/* No data transfer */
-	if (!scsi_bufflen(cmd) || cmd->sc_data_direction == DMA_NONE) {
+	if (!scsi_bufflen(cmd) || cmd->sc_data_direction == DMA_NONE ||
+	    tot_dsds == 0) {
  		cmd_pkt->byte_count = cpu_to_le32(0);
  		return 0;
  	}

Is the above change sufficient to suppress the Klocwork warning?

Thanks,

Bart.

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* RE: [EXT] Re: [PATCH 2/8] qla2xxx: klocwork - Fix potential null pointer dereference
  2023-05-31 12:33       ` Bart Van Assche
@ 2023-06-07  8:31         ` Nilesh Javali
  0 siblings, 0 replies; 15+ messages in thread
From: Nilesh Javali @ 2023-06-07  8:31 UTC (permalink / raw)
  To: Bart Van Assche, martin.petersen
  Cc: linux-scsi, GR-QLogic-Storage-Upstream, Bikash Hazarika,
	Anil Gurumurthy, Shreyas Deodhar



> -----Original Message-----
> From: Bart Van Assche <bvanassche@acm.org>
> Sent: Wednesday, May 31, 2023 6:04 PM
> To: Nilesh Javali <njavali@marvell.com>; martin.petersen@oracle.com
> Cc: linux-scsi@vger.kernel.org; GR-QLogic-Storage-Upstream <GR-QLogic-
> Storage-Upstream@marvell.com>; Bikash Hazarika <bhazarika@marvell.com>;
> Anil Gurumurthy <agurumurthy@marvell.com>; Shreyas Deodhar
> <sdeodhar@marvell.com>
> Subject: Re: [EXT] Re: [PATCH 2/8] qla2xxx: klocwork - Fix potential null pointer
> dereference
> 
> On 5/31/23 04:43, Nilesh Javali wrote:
> > We can prevent the crash and notify the occurrence of this
> > rare case by adding warn_on like,
> >
> > +       WARN_ON_ONCE(!cur_dsd);
> > +       if (cur_dsd) {
> > +               cur_dsd->address = 0;
> > +               cur_dsd->length = 0;
> > +               cur_dsd++;
> > +       }
> >          cmd_pkt->control_flags |= cpu_to_le16(CF_DATA_SEG_DESCR_ENABLE);
> >          return 0;
> >   }
> 
> I think there is a much better solution: drop the new "if (cur_dsd) {"
> test and instead add the following code:
> 
> diff --git a/drivers/scsi/qla2xxx/qla_iocb.c
> b/drivers/scsi/qla2xxx/qla_iocb.c
> index 6acfdcc48b16..a1675f056a5c 100644
> --- a/drivers/scsi/qla2xxx/qla_iocb.c
> +++ b/drivers/scsi/qla2xxx/qla_iocb.c
> @@ -607,7 +607,8 @@ qla24xx_build_scsi_type_6_iocbs(srb_t *sp, struct
> cmd_type_6 *cmd_pkt,
>   	put_unaligned_le32(COMMAND_TYPE_6, &cmd_pkt->entry_type);
> 
>   	/* No data transfer */
> -	if (!scsi_bufflen(cmd) || cmd->sc_data_direction == DMA_NONE) {
> +	if (!scsi_bufflen(cmd) || cmd->sc_data_direction == DMA_NONE ||
> +	    tot_dsds == 0) {
>   		cmd_pkt->byte_count = cpu_to_le32(0);
>   		return 0;
>   	}
> 
> Is the above change sufficient to suppress the Klocwork warning?

This looks good and does not generate any new klocwork warning.
I will re-send the series with this change included.

Thanks,
Nilesh

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2023-06-07  8:31 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-18  7:58 [PATCH 0/8] qla2xxx klocwork fixes Nilesh Javali
2023-05-18  7:58 ` [PATCH 1/8] qla2xxx: klocwork - Array index may go out of bound Nilesh Javali
2023-05-18  7:58 ` [PATCH 2/8] qla2xxx: klocwork - Fix potential null pointer dereference Nilesh Javali
2023-05-18 17:42   ` Bart Van Assche
2023-05-31 11:43     ` [EXT] " Nilesh Javali
2023-05-31 12:33       ` Bart Van Assche
2023-06-07  8:31         ` Nilesh Javali
2023-05-18  7:58 ` [PATCH 3/8] qla2xxx: klocwork - Check for a valid fcport pointer Nilesh Javali
2023-05-18 17:44   ` Bart Van Assche
2023-05-18  7:58 ` [PATCH 4/8] qla2xxx: klocwork - Check valid rport returned by fc_bsg_to_rport Nilesh Javali
2023-05-18  7:58 ` [PATCH 5/8] qla2xxx: klocwork - Fix buffer overrun Nilesh Javali
2023-05-18  7:58 ` [PATCH 6/8] qla2xxx: klocwork - pointer may be dereferenced Nilesh Javali
2023-05-18  7:58 ` [PATCH 7/8] qla2xxx: klocwork - correct the index of array Nilesh Javali
2023-05-18 17:47   ` Bart Van Assche
2023-05-18  7:58 ` [PATCH 8/8] qla2xxx: Update version to 10.02.08.400-k Nilesh Javali

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.