diff mbox series

[2/8] qla2xxx: klocwork - Fix potential null pointer dereference

Message ID 20230518075841.40363-3-njavali@marvell.com (mailing list archive)
State Superseded
Headers show
Series qla2xxx klocwork fixes | expand

Commit Message

Nilesh Javali May 18, 2023, 7:58 a.m. UTC 
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(-)

Comments

Bart Van Assche May 18, 2023, 5:42 p.m. UTC | #1 
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.
Nilesh Javali May 31, 2023, 11:43 a.m. UTC | #2 
> -----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
Bart Van Assche May 31, 2023, 12:33 p.m. UTC | #3 
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.
Nilesh Javali June 7, 2023, 8:31 a.m. UTC | #4 
> -----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
diff mbox series

Patch

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;
 }