* [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.