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

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(-)

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

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.