[patch] bfa: clean up some bounds checking

[patch] bfa: clean up some bounds checking

[Dan Carpenter]

This code is supposed to search ->adapter_hwpath[] and replace the
second colon with a NUL character.  Unfortunately, the boundary checks
that ensure we don't go beyond the end of the buffer have a couple
problems.

Imagine that the string has no colons.  In that case, in the first loop,
we read one space beyond the end of the buffer and then exit the loop.
In the next loop, we increment once, read two characters beyond the end
of the buffer and then exit.  Then after the loop we put a NUL character
two characters past the end of the buffer.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
This is from static analysis and not tested.  Caveat emptor.

diff --git a/drivers/scsi/bfa/bfad_bsg.c b/drivers/scsi/bfa/bfad_bsg.c
index d1ad020..dfb26f0 100644
--- a/drivers/scsi/bfa/bfad_bsg.c
+++ b/drivers/scsi/bfa/bfad_bsg.c
@@ -106,10 +106,17 @@ bfad_iocmd_ioc_get_info(struct bfad_s *bfad, void *cmd)
 
 	/* set adapter hw path */
 	strcpy(iocmd->adapter_hwpath, bfad->pci_name);
-	for (i = 0; iocmd->adapter_hwpath[i] != ':' && i < BFA_STRING_32; i++)
-		;
-	for (; iocmd->adapter_hwpath[++i] != ':' && i < BFA_STRING_32; )
-		;
+	i = -1;
+	while (++i < BFA_STRING_32) {
+		if (iocmd->adapter_hwpath[i] = ':')
+			break;
+	}
+	while (++i < BFA_STRING_32) {
+		if (iocmd->adapter_hwpath[i] = ':')
+			break;
+	}
+	if (i >= BFA_STRING_32)
+		i = BFA_STRING_32 - 1;
 	iocmd->adapter_hwpath[i] = '\0';
 	iocmd->status = BFA_STATUS_OK;
 	return 0;

Re: [patch] bfa: clean up some bounds checking

[walter harms]

Am 16.06.2016 12:44, schrieb Dan Carpenter:
> This code is supposed to search ->adapter_hwpath[] and replace the
> second colon with a NUL character.  Unfortunately, the boundary checks
> that ensure we don't go beyond the end of the buffer have a couple
> problems.
> 
> Imagine that the string has no colons.  In that case, in the first loop,
> we read one space beyond the end of the buffer and then exit the loop.
> In the next loop, we increment once, read two characters beyond the end
> of the buffer and then exit.  Then after the loop we put a NUL character
> two characters past the end of the buffer.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> This is from static analysis and not tested.  Caveat emptor.
> 
> diff --git a/drivers/scsi/bfa/bfad_bsg.c b/drivers/scsi/bfa/bfad_bsg.c
> index d1ad020..dfb26f0 100644
> --- a/drivers/scsi/bfa/bfad_bsg.c
> +++ b/drivers/scsi/bfa/bfad_bsg.c
> @@ -106,10 +106,17 @@ bfad_iocmd_ioc_get_info(struct bfad_s *bfad, void *cmd)
>  
>  	/* set adapter hw path */
>  	strcpy(iocmd->adapter_hwpath, bfad->pci_name);
> -	for (i = 0; iocmd->adapter_hwpath[i] != ':' && i < BFA_STRING_32; i++)
> -		;
> -	for (; iocmd->adapter_hwpath[++i] != ':' && i < BFA_STRING_32; )
> -		;
> +	i = -1;
> +	while (++i < BFA_STRING_32) {
> +		if (iocmd->adapter_hwpath[i] = ':')
> +			break;
> +	}
> +	while (++i < BFA_STRING_32) {
> +		if (iocmd->adapter_hwpath[i] = ':')
> +			break;
> +	}
> +	if (i >= BFA_STRING_32)
> +		i = BFA_STRING_32 - 1;
>  	iocmd->adapter_hwpath[i] = '\0';
>  	iocmd->status = BFA_STATUS_OK;
>  	return 0;


I do not see the use case but i assume
the idea is to have a string like aa:bb:something
and kill everyhing after the second ':' ?
/*
	a few word may help here also inside the code
*/


second: maybe we can us strchr here ?
	s1=strchr(iocmd->adapter_hwpath,':');
	if (s1 != NULL ) s1=strchr(s1,":");



re,
 wh

RE: [patch] bfa: clean up some bounds checking

[Sudarsana Kalluru]

    Both the approaches (i.e., using a loop or the strchr()) look fine to me.

Thanks,
Sudarsana

-----Original Message-----
From: walter harms [mailto:wharms@bfs.de] 
Sent: 16 June 2016 17:50
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Anil Gurumurthy <Anil.Gurumurthy@qlogic.com>; Sudarsana Kalluru <Sudarsana.Kalluru@qlogic.com>; James E.J. Bottomley <jejb@linux.vnet.ibm.com>; Martin K. Petersen <martin.petersen@oracle.com>; linux-scsi <linux-scsi@vger.kernel.org>; kernel-janitors@vger.kernel.org
Subject: Re: [patch] bfa: clean up some bounds checking



Am 16.06.2016 12:44, schrieb Dan Carpenter:
> This code is supposed to search ->adapter_hwpath[] and replace the 
> second colon with a NUL character.  Unfortunately, the boundary checks 
> that ensure we don't go beyond the end of the buffer have a couple 
> problems.
> 
> Imagine that the string has no colons.  In that case, in the first 
> loop, we read one space beyond the end of the buffer and then exit the loop.
> In the next loop, we increment once, read two characters beyond the 
> end of the buffer and then exit.  Then after the loop we put a NUL 
> character two characters past the end of the buffer.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> This is from static analysis and not tested.  Caveat emptor.
> 
> diff --git a/drivers/scsi/bfa/bfad_bsg.c b/drivers/scsi/bfa/bfad_bsg.c 
> index d1ad020..dfb26f0 100644
> --- a/drivers/scsi/bfa/bfad_bsg.c
> +++ b/drivers/scsi/bfa/bfad_bsg.c
> @@ -106,10 +106,17 @@ bfad_iocmd_ioc_get_info(struct bfad_s *bfad, 
> void *cmd)
>  
>  	/* set adapter hw path */
>  	strcpy(iocmd->adapter_hwpath, bfad->pci_name);
> -	for (i = 0; iocmd->adapter_hwpath[i] != ':' && i < BFA_STRING_32; i++)
> -		;
> -	for (; iocmd->adapter_hwpath[++i] != ':' && i < BFA_STRING_32; )
> -		;
> +	i = -1;
> +	while (++i < BFA_STRING_32) {
> +		if (iocmd->adapter_hwpath[i] = ':')
> +			break;
> +	}
> +	while (++i < BFA_STRING_32) {
> +		if (iocmd->adapter_hwpath[i] = ':')
> +			break;
> +	}
> +	if (i >= BFA_STRING_32)
> +		i = BFA_STRING_32 - 1;
>  	iocmd->adapter_hwpath[i] = '\0';
>  	iocmd->status = BFA_STATUS_OK;
>  	return 0;


I do not see the use case but i assume
the idea is to have a string like aa:bb:something and kill everyhing after the second ':' ?
/*
	a few word may help here also inside the code */


second: maybe we can us strchr here ?
	s1=strchr(iocmd->adapter_hwpath,':');
	if (s1 != NULL ) s1=strchr(s1,":");



re,
 wh