[patch] scsi_dh_alua: uninitialized variable in alua_rtpg()

[patch] scsi_dh_alua: uninitialized variable in alua_rtpg()

[Dan Carpenter]

It's possible to use "err" without initializing it.  If it happens to be
a 2 which is SCSI_DH_RETRY then that could cause a bug.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 8eaed05..f3c994f 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -513,7 +513,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 	struct alua_port_group *tmp_pg;
 	int len, k, off, valid_states = 0, bufflen = ALUA_RTPG_SIZE;
 	unsigned char *desc, *buff;
-	unsigned err, retval;
+	unsigned int err = 0;
+	unsigned int retval;
 	unsigned int tpg_desc_tbl_off;
 	unsigned char orig_transition_tmo;
 	unsigned long flags;

Re: [patch] scsi_dh_alua: uninitialized variable in alua_rtpg()

[Bart Van Assche]

On 04/14/2016 02:39 AM, Dan Carpenter wrote:
> It's possible to use "err" without initializing it.  If it happens to be
> a 2 which is SCSI_DH_RETRY then that could cause a bug.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
> index 8eaed05..f3c994f 100644
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -513,7 +513,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
>   	struct alua_port_group *tmp_pg;
>   	int len, k, off, valid_states = 0, bufflen = ALUA_RTPG_SIZE;
>   	unsigned char *desc, *buff;
> -	unsigned err, retval;
> +	unsigned int err = 0;
> +	unsigned int retval;
>   	unsigned int tpg_desc_tbl_off;
>   	unsigned char orig_transition_tmo;
>   	unsigned long flags;

Hello Dan,

The code that uses the 'err' variable occurs in a loop. I think the 
initialization of 'err' should occur after the "retry:" label.

Bart.

[patch v2] scsi_dh_alua: uninitialized variable in alua_rtpg()

[Dan Carpenter]

It's possible to use "err" without initializing it.  If it happens to be
a 2 which is SCSI_DH_RETRY then that could cause a bug.  Bart Van Assche
pointed out that we should probably re-initialize it for every iteration
through the retry loop.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: The first version just initialized it at the start of the function.

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 8eaed05..a655cf2 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -532,6 +532,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 		return SCSI_DH_DEV_TEMP_BUSY;
 
  retry:
+	err = 0;
 	retval = submit_rtpg(sdev, buff, bufflen, &sense_hdr, pg->flags);
 
 	if (retval) {

Re: [patch] scsi_dh_alua: uninitialized variable in alua_rtpg()

[Dan Carpenter]

On Thu, Apr 14, 2016 at 08:45:18AM -0700, Bart Van Assche wrote:
> On 04/14/2016 02:39 AM, Dan Carpenter wrote:
> >It's possible to use "err" without initializing it.  If it happens to be
> >a 2 which is SCSI_DH_RETRY then that could cause a bug.
> >
> >Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> >diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
> >index 8eaed05..f3c994f 100644
> >--- a/drivers/scsi/device_handler/scsi_dh_alua.c
> >+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> >@@ -513,7 +513,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
> >  	struct alua_port_group *tmp_pg;
> >  	int len, k, off, valid_states = 0, bufflen = ALUA_RTPG_SIZE;
> >  	unsigned char *desc, *buff;
> >-	unsigned err, retval;
> >+	unsigned int err = 0;
> >+	unsigned int retval;
> >  	unsigned int tpg_desc_tbl_off;
> >  	unsigned char orig_transition_tmo;
> >  	unsigned long flags;
> 
> Hello Dan,
> 
> The code that uses the 'err' variable occurs in a loop. I think the
> initialization of 'err' should occur after the "retry:" label.

It looks like you're right.  I'll resend.  I don't know this code very
well, obviously and it's a static checker fix not something I have
tested.

regards,
dan carpenter

Re: [patch v2] scsi_dh_alua: uninitialized variable in alua_rtpg()

[Bart Van Assche]

On 04/14/2016 11:20 AM, Dan Carpenter wrote:
> It's possible to use "err" without initializing it.  If it happens to be
> a 2 which is SCSI_DH_RETRY then that could cause a bug.  Bart Van Assche
> pointed out that we should probably re-initialize it for every iteration
> through the retry loop.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: The first version just initialized it at the start of the function.
>
> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
> index 8eaed05..a655cf2 100644
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -532,6 +532,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
>   		return SCSI_DH_DEV_TEMP_BUSY;
>
>    retry:
> +	err = 0;
>   	retval = submit_rtpg(sdev, buff, bufflen, &sense_hdr, pg->flags);
>
>   	if (retval) {

Although I would have preferred that that initialization would have been 
closer to the other 'err' assignments this patch looks fine to me. If 
this patch does not get integrated in kernel v4.6 a "Cc: stable" tag 
will be needed.

Bart.

Re: [patch v2] scsi_dh_alua: uninitialized variable in alua_rtpg()

[Hannes Reinecke]

On 04/14/2016 08:20 PM, Dan Carpenter wrote:
> It's possible to use "err" without initializing it.  If it happens to be
> a 2 which is SCSI_DH_RETRY then that could cause a bug.  Bart Van Assche
> pointed out that we should probably re-initialize it for every iteration
> through the retry loop.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: The first version just initialized it at the start of the function.
> 
> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
> index 8eaed05..a655cf2 100644
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -532,6 +532,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
>  		return SCSI_DH_DEV_TEMP_BUSY;
>  
>   retry:
> +	err = 0;
>  	retval = submit_rtpg(sdev, buff, bufflen, &sense_hdr, pg->flags);
>  
>  	if (retval) {
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [patch v2] scsi_dh_alua: uninitialized variable in alua_rtpg()

[Martin K. Petersen]

>>>>> "Dan" == Dan Carpenter <dan.carpenter@oracle.com> writes:

Dan> It's possible to use "err" without initializing it.  If it happens
Dan> to be a 2 which is SCSI_DH_RETRY then that could cause a bug.  Bart
Dan> Van Assche pointed out that we should probably re-initialize it for
Dan> every iteration through the retry loop.

Applied to 4.6/scsi-fixes.

-- 
Martin K. Petersen	Oracle Linux Engineering