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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Fri, 6 Jun 2014 15:02:30 +0200
From:	Rickard Strandqvist <rickard_strandqvist@...ctrumdigital.se>
To:	James Bottomley <James.Bottomley@...senpartnership.com>
Cc:	Nagalakshmi Nandigama <Nagalakshmi.Nandigama@....com>,
	Sreekanth Reddy <Sreekanth.Reddy@....com>, support@....com,
	DL-MPTFusionLinux@....com, linux-scsi@...r.kernel.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] scsi: mpt2sas: mpt2sas_base.c: Cleaning up missing
 null-terminate after strncpy call

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@...senpartnership.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@...ctrumdigital.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
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/