[PATCH] scsi: mpt2sas: mpt2sas_base.c: Cleaning up missing null-terminate after strncpy call

[PATCH] scsi: mpt2sas: mpt2sas_base.c: Cleaning up missing null-terminate after strncpy call

[Rickard Strandqvist]

Added a guaranteed null-terminate after call to strncpy.

This was partly found using a static code analysis program called cppcheck.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 drivers/scsi/mpt2sas/mpt2sas_base.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c b/drivers/scsi/mpt2sas/mpt2sas_base.c
index bde63f7..4c3eceb 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_base.c
+++ b/drivers/scsi/mpt2sas/mpt2sas_base.c
@@ -2097,7 +2097,8 @@ _base_display_ioc_capabilities(struct MPT2SAS_ADAPTER *ioc)
 	u32 bios_version;
 
 	bios_version = le32_to_cpu(ioc->bios_pg3.BiosVersion);
-	strncpy(desc, ioc->manu_pg0.ChipName, 16);
+	strncpy(desc, ioc->manu_pg0.ChipName, sizeof(desc));
+	desc[sizeof(desc) - 1] = '\0';
 	printk(MPT2SAS_INFO_FMT "%s: FWVersion(%02d.%02d.%02d.%02d), "
 	   "ChipRevision(0x%02x), BiosVersion(%02d.%02d.%02d.%02d)\n",
 	    ioc->name, desc,
-- 
1.7.10.4

Re: [PATCH] scsi: mpt2sas: mpt2sas_base.c: Cleaning up missing null-terminate after strncpy call

[Joe Perches]

On Wed, 2014-06-04 at 23:36 +0200, Rickard Strandqvist wrote:
> Added a guaranteed null-terminate after call to strncpy.

Next time you submit a patch series like this
can you please use a cover letter?

That way I can reply to the cover letter with
a comment about the series instead of multiple
replies to various patches.

I think all of these should be using strlcpy not
strncpy.

> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c b/drivers/scsi/mpt2sas/mpt2sas_base.c
[]
> @@ -2097,7 +2097,8 @@ _base_display_ioc_capabilities(struct MPT2SAS_ADAPTER *ioc)
>  	u32 bios_version;
>  
>  	bios_version = le32_to_cpu(ioc->bios_pg3.BiosVersion);
> -	strncpy(desc, ioc->manu_pg0.ChipName, 16);
> +	strncpy(desc, ioc->manu_pg0.ChipName, sizeof(desc));
> +	desc[sizeof(desc) - 1] = '\0';

Re: [PATCH] scsi: mpt2sas: mpt2sas_base.c: Cleaning up missing null-terminate after strncpy call

[Rickard Strandqvist]

Hi

A little embarrassing, but I actually did not know that there was a
better replacement for strncpy.

Sorry, but I will send a new platch based on strlcpy instead then.

Will investigate cover letter then to.


Best regards
Rickard Strandqvist


2014-06-05 0:01 GMT+02:00 Joe Perches <joe@perches.com>:
> On Wed, 2014-06-04 at 23:36 +0200, Rickard Strandqvist wrote:
>> Added a guaranteed null-terminate after call to strncpy.
>
> Next time you submit a patch series like this
> can you please use a cover letter?
>
> That way I can reply to the cover letter with
> a comment about the series instead of multiple
> replies to various patches.
>
> I think all of these should be using strlcpy not
> strncpy.
>
>> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c b/drivers/scsi/mpt2sas/mpt2sas_base.c
> []
>> @@ -2097,7 +2097,8 @@ _base_display_ioc_capabilities(struct MPT2SAS_ADAPTER *ioc)
>>       u32 bios_version;
>>
>>       bios_version = le32_to_cpu(ioc->bios_pg3.BiosVersion);
>> -     strncpy(desc, ioc->manu_pg0.ChipName, 16);
>> +     strncpy(desc, ioc->manu_pg0.ChipName, sizeof(desc));
>> +     desc[sizeof(desc) - 1] = '\0';
>
>

Re: [PATCH] scsi: mpt2sas: mpt2sas_base.c: Cleaning up missing null-terminate after strncpy call

[James Bottomley]

On Wed, 2014-06-04 at 23:36 +0200, Rickard Strandqvist wrote:
> Added a guaranteed null-terminate after call to strncpy.
> 
> This was partly found using a static code analysis program called cppcheck.
> 
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ---
>  drivers/scsi/mpt2sas/mpt2sas_base.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c b/drivers/scsi/mpt2sas/mpt2sas_base.c
> index bde63f7..4c3eceb 100644
> --- a/drivers/scsi/mpt2sas/mpt2sas_base.c
> +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c
> @@ -2097,7 +2097,8 @@ _base_display_ioc_capabilities(struct MPT2SAS_ADAPTER *ioc)
>  	u32 bios_version;
>  
>  	bios_version = le32_to_cpu(ioc->bios_pg3.BiosVersion);
> -	strncpy(desc, ioc->manu_pg0.ChipName, 16);
> +	strncpy(desc, ioc->manu_pg0.ChipName, sizeof(desc));
> +	desc[sizeof(desc) - 1] = '\0';

There's no bug here because the specs define the ChipName field of the
manufacturing page 0 to be 16 bytes and null terminated.  The nasty part
is the way this driver is littered with magic numbers.

James

Re: [PATCH] scsi: mpt2sas: mpt2sas_base.c: Cleaning up missing null-terminate after strncpy call

[Rickard Strandqvist]

Hi

I did not know that Linux had strlcpy, much better choice, of course!

It was not clear that this was done in the code. And my thought was,
rather, just that you were not sure if there was a null character
because it used strlcpy. if this can not possibly happen would be
almost as well to switch to strcpy outright.

So what happens now?
Should I make a new patch, with strlcpy or just straight off with strcpy then?



Best regards
Rickard Strandqvist


2014-06-05 0:28 GMT+02:00 James Bottomley
<James.Bottomley@hansenpartnership.com>:
> On Wed, 2014-06-04 at 23:36 +0200, Rickard Strandqvist wrote:
>> Added a guaranteed null-terminate after call to strncpy.
>>
>> This was partly found using a static code analysis program called cppcheck.
>>
>> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
>> ---
>>  drivers/scsi/mpt2sas/mpt2sas_base.c |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c b/drivers/scsi/mpt2sas/mpt2sas_base.c
>> index bde63f7..4c3eceb 100644
>> --- a/drivers/scsi/mpt2sas/mpt2sas_base.c
>> +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c
>> @@ -2097,7 +2097,8 @@ _base_display_ioc_capabilities(struct MPT2SAS_ADAPTER *ioc)
>>       u32 bios_version;
>>
>>       bios_version = le32_to_cpu(ioc->bios_pg3.BiosVersion);
>> -     strncpy(desc, ioc->manu_pg0.ChipName, 16);
>> +     strncpy(desc, ioc->manu_pg0.ChipName, sizeof(desc));
>> +     desc[sizeof(desc) - 1] = '\0';
>
> There's no bug here because the specs define the ChipName field of the
> manufacturing page 0 to be 16 bytes and null terminated.  The nasty part
> is the way this driver is littered with magic numbers.
>
> James
>
>