diff mbox

bfa: clean up some bounds checking

Message ID 20160616104434.GB24067@mwanda (mailing list archive)
State Changes Requested, archived
Headers show

Commit Message

Dan Carpenter June 16, 2016, 10:44 a.m. UTC 
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Walter Harms June 16, 2016, 12:20 p.m. UTC | #1 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sudarsana Kalluru July 1, 2016, 1:12 p.m. UTC | #2 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;