mptfusion: use strlcpy() instead of strncpy()

Message ID 1515757619-29651-1-git-send-email-wangxiongfeng2@huawei.com
State New
Headers show
Series
  • mptfusion: use strlcpy() instead of strncpy()
Related show

Commit Message

Xiongfeng Wang Jan. 12, 2018, 11:46 a.m.
From: Xiongfeng Wang <xiongfeng.wang@linaro.org>


drivers/message/fusion/mptctl.c: In function '__mptctl_ioctl.isra.3':
./include/linux/string.h:245:9: warning: '__builtin_strncpy' specified
bound 12 equals destination size [-Wstringop-truncation]

The compiler requires that the destination size should be greater than
the length we copy to make sure the dest string is nul-terminated. We
can just use strlcpy() to avoid this warning.

Signed-off-by: Xiongfeng Wang <xiongfeng.wang@linaro.org>

---
 drivers/message/fusion/mptctl.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

-- 
1.8.3.1

Comments

Andy Shevchenko Jan. 12, 2018, 1:45 p.m. | #1
On Fri, Jan 12, 2018 at 1:46 PM, Xiongfeng Wang
<wangxiongfeng2@huawei.com> wrote:
> From: Xiongfeng Wang <xiongfeng.wang@linaro.org>

>

> drivers/message/fusion/mptctl.c: In function '__mptctl_ioctl.isra.3':

> ./include/linux/string.h:245:9: warning: '__builtin_strncpy' specified

> bound 12 equals destination size [-Wstringop-truncation]

>

> The compiler requires that the destination size should be greater than

> the length we copy to make sure the dest string is nul-terminated. We

> can just use strlcpy() to avoid this warning.


Are you sure it's a best approach in this case?

> -       strncpy (karg->driverVersion, MPT_LINUX_PACKAGE_NAME, MPT_IOCTL_VERSION_LENGTH);

> -       karg->driverVersion[MPT_IOCTL_VERSION_LENGTH-1]='\0';

> +       strlcpy (karg->driverVersion, MPT_LINUX_PACKAGE_NAME, MPT_IOCTL_VERSION_LENGTH);


This one is false positive.

> -       strncpy (karg.name, ioc->name, MPT_MAX_NAME);

> -       karg.name[MPT_MAX_NAME-1]='\0';

> -       strncpy (karg.product, ioc->prod_name, MPT_PRODUCT_LENGTH);

> -       karg.product[MPT_PRODUCT_LENGTH-1]='\0';

> +       strlcpy (karg.name, ioc->name, MPT_MAX_NAME);

> +       strlcpy (karg.product, ioc->prod_name, MPT_PRODUCT_LENGTH);


These two as well.

> -       strncpy(karg.serial_number, " ", 24);

> +       strlcpy(karg.serial_number, " ", 24);


This one is interesting indeed.
Though the fix would be rather something like

memset(&karg.serial_number, " ", 24); // leave 24 for best performance of memset
karg.serial_number[24-1] = '\0';

>                                 if (mpt_config(ioc, &cfg) == 0) {

>                                         ManufacturingPage0_t *pdata = (ManufacturingPage0_t *) pbuf;


>                                         if (strlen(pdata->BoardTracerNumber) > 1) {

> -                                               strncpy(karg.serial_number,                                                                         pdata->BoardTracerNumber, 24);

> -                                               karg.serial_number[24-1]='\0';

> +                                               strlcpy(karg.serial_number,

> +                                                       pdata->BoardTracerNumber, 24);

>                                         }


...and here you don't need to touch anything.

-- 
With Best Regards,
Andy Shevchenko
Bart Van Assche Jan. 12, 2018, 3:09 p.m. | #2
On Fri, 2018-01-12 at 15:45 +0200, Andy Shevchenko wrote:
> On Fri, Jan 12, 2018 at 1:46 PM,  Wang <wangxiongfeng2@huawei.com> wrote:

> > 

> > -       strncpy (karg->driverVersion, MPT_LINUX_PACKAGE_NAME, MPT_IOCTL_VERSION_LENGTH);

> > -       karg->driverVersion[MPT_IOCTL_VERSION_LENGTH-1]='\0';

> > +       strlcpy (karg->driverVersion, MPT_LINUX_PACKAGE_NAME, MPT_IOCTL_VERSION_LENGTH);

> 

> This one is false positive.

> 

> > -       strncpy (karg.name, ioc->name, MPT_MAX_NAME);

> > -       karg.name[MPT_MAX_NAME-1]='\0';

> > -       strncpy (karg.product, ioc->prod_name, MPT_PRODUCT_LENGTH);

> > -       karg.product[MPT_PRODUCT_LENGTH-1]='\0';

> > +       strlcpy (karg.name, ioc->name, MPT_MAX_NAME);

> > +       strlcpy (karg.product, ioc->prod_name, MPT_PRODUCT_LENGTH);

> 

> These two as well.


But using strlcpy() makes the code easier to read.

Xiongfeng, if you want to continue with this patch, please change the third
argument of all strlcpy() calls into a sizeof() expression. That make the
code easier to verify.

> > -       strncpy(karg.serial_number, " ", 24);

> > +       strlcpy(karg.serial_number, " ", 24);

> 

> This one is interesting indeed.

> Though the fix would be rather something like

> 

> memset(&karg.serial_number, " ", 24); // leave 24 for best performance of memset

> karg.serial_number[24-1] = '\0';


Does performance really matter in this context? How about the following instead:

snprintf(karg.serial_number, sizeof(karg.serial_number), "%*s", (int)(karg.serial_number) - 1, "");

Bart.
Andy Shevchenko Jan. 15, 2018, 7:34 p.m. | #3
On Fri, Jan 12, 2018 at 5:09 PM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> On Fri, 2018-01-12 at 15:45 +0200, Andy Shevchenko wrote:

>> On Fri, Jan 12, 2018 at 1:46 PM,  Wang <wangxiongfeng2@huawei.com> wrote:


>> This one is false positive.



>> > +       strlcpy (karg.name, ioc->name, MPT_MAX_NAME);

>> > +       strlcpy (karg.product, ioc->prod_name, MPT_PRODUCT_LENGTH);

>>

>> These two as well.

>

> But using strlcpy() makes the code easier to read.


This is another story, not mentioned in the original commit message.

> Xiongfeng, if you want to continue with this patch, please change the third

> argument of all strlcpy() calls into a sizeof() expression. That make the

> code easier to verify.

>

>> > -       strncpy(karg.serial_number, " ", 24);

>> > +       strlcpy(karg.serial_number, " ", 24);

>>

>> This one is interesting indeed.

>> Though the fix would be rather something like

>>

>> memset(&karg.serial_number, " ", 24); // leave 24 for best performance of memset

>> karg.serial_number[24-1] = '\0';

>

> Does performance really matter in this context?


Nope, but still good to understand this aspect. On some architectures
unaligned access is expensive.

> How about the following instead:

>

> snprintf(karg.serial_number, sizeof(karg.serial_number), "%*s", (int)(karg.serial_number) - 1, "");


...and you is talking about "easy to read"?!

So, my view on the patch is:
1) fix a real issue w/o touching everything around;
2) (optionally) move to strlcpy() with a correct selling point.

-- 
With Best Regards,
Andy Shevchenko
Xiongfeng Wang Jan. 16, 2018, 11:06 a.m. | #4
On 2018/1/16 3:34, Andy Shevchenko wrote:
> On Fri, Jan 12, 2018 at 5:09 PM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:

>> On Fri, 2018-01-12 at 15:45 +0200, Andy Shevchenko wrote:

>>> On Fri, Jan 12, 2018 at 1:46 PM,  Wang <wangxiongfeng2@huawei.com> wrote:

> 

>>> This one is false positive.

> 

> 

>>>> +       strlcpy (karg.name, ioc->name, MPT_MAX_NAME);

>>>> +       strlcpy (karg.product, ioc->prod_name, MPT_PRODUCT_LENGTH);

>>>

>>> These two as well.

>>

>> But using strlcpy() makes the code easier to read.

> 

> This is another story, not mentioned in the original commit message.

> 

>> Xiongfeng, if you want to continue with this patch, please change the third

>> argument of all strlcpy() calls into a sizeof() expression. That make the

>> code easier to verify.

>>

>>>> -       strncpy(karg.serial_number, " ", 24);

>>>> +       strlcpy(karg.serial_number, " ", 24);

>>>

>>> This one is interesting indeed.

>>> Though the fix would be rather something like

>>>

>>> memset(&karg.serial_number, " ", 24); // leave 24 for best performance of memset

>>> karg.serial_number[24-1] = '\0';

>>

>> Does performance really matter in this context?

> 

> Nope, but still good to understand this aspect. On some architectures

> unaligned access is expensive.

> 

>> How about the following instead:

>>

>> snprintf(karg.serial_number, sizeof(karg.serial_number), "%*s", (int)(karg.serial_number) - 1, "");

> 

> ...and you is talking about "easy to read"?!

> 

> So, my view on the patch is:

> 1) fix a real issue w/o touching everything around;



> 2) (optionally) move to strlcpy() with a correct selling point.

> 


The code is right. There are no issues. It's just that the new compiler gcc-8 prints the warnings.
I use strlcpy() instead of strncpy() just to suppress the warnings.

Thanks,
Xiongfeng

Patch

diff --git a/drivers/message/fusion/mptctl.c b/drivers/message/fusion/mptctl.c
index 7b3b413..15f2aca 100644
--- a/drivers/message/fusion/mptctl.c
+++ b/drivers/message/fusion/mptctl.c
@@ -1353,8 +1353,7 @@  static int mptctl_do_reset(unsigned long arg)
 
 	/* Set the Version Strings.
 	 */
-	strncpy (karg->driverVersion, MPT_LINUX_PACKAGE_NAME, MPT_IOCTL_VERSION_LENGTH);
-	karg->driverVersion[MPT_IOCTL_VERSION_LENGTH-1]='\0';
+	strlcpy (karg->driverVersion, MPT_LINUX_PACKAGE_NAME, MPT_IOCTL_VERSION_LENGTH);
 
 	karg->busChangeEvent = 0;
 	karg->hostId = ioc->pfacts[port].PortSCSIID;
@@ -1542,10 +1541,8 @@  static int mptctl_do_reset(unsigned long arg)
 #else
 	karg.chip_type = ioc->pcidev->device;
 #endif
-	strncpy (karg.name, ioc->name, MPT_MAX_NAME);
-	karg.name[MPT_MAX_NAME-1]='\0';
-	strncpy (karg.product, ioc->prod_name, MPT_PRODUCT_LENGTH);
-	karg.product[MPT_PRODUCT_LENGTH-1]='\0';
+	strlcpy (karg.name, ioc->name, MPT_MAX_NAME);
+	strlcpy (karg.product, ioc->prod_name, MPT_PRODUCT_LENGTH);
 
 	/* Copy the data from kernel memory to user memory
 	 */
@@ -2513,7 +2510,7 @@  static int mptctl_do_reset(unsigned long arg)
 	cfg.dir = 0;	/* read */
 	cfg.timeout = 10;
 
-	strncpy(karg.serial_number, " ", 24);
+	strlcpy(karg.serial_number, " ", 24);
 	if (mpt_config(ioc, &cfg) == 0) {
 		if (cfg.cfghdr.hdr->PageLength > 0) {
 			/* Issue the second config page request */
@@ -2525,8 +2522,8 @@  static int mptctl_do_reset(unsigned long arg)
 				if (mpt_config(ioc, &cfg) == 0) {
 					ManufacturingPage0_t *pdata = (ManufacturingPage0_t *) pbuf;
 					if (strlen(pdata->BoardTracerNumber) > 1) {
-						strncpy(karg.serial_number, 									    pdata->BoardTracerNumber, 24);
-						karg.serial_number[24-1]='\0';
+						strlcpy(karg.serial_number,
+							pdata->BoardTracerNumber, 24);
 					}
 				}
 				pci_free_consistent(ioc->pcidev, hdr.PageLength * 4, pbuf, buf_dma);