| Message ID | 1591241524-6452-1-git-send-email-zhangfei.gao@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: hisilicon - fix strncpy warning with strlcpy | expand |
On Thu, Jun 04, 2020 at 11:32:04AM +0800, Zhangfei Gao wrote: > Use strlcpy to fix the warning > warning: 'strncpy' specified bound 64 equals destination size > [-Wstringop-truncation] > > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> > --- > drivers/crypto/hisilicon/qm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/crypto/hisilicon/qm.c b/drivers/crypto/hisilicon/qm.c > index f795fb5..224f3e2 100644 > --- a/drivers/crypto/hisilicon/qm.c > +++ b/drivers/crypto/hisilicon/qm.c > @@ -1574,7 +1574,7 @@ static int qm_alloc_uacce(struct hisi_qm *qm) > .ops = &uacce_qm_ops, > }; > > - strncpy(interface.name, pdev->driver->name, sizeof(interface.name)); > + strlcpy(interface.name, pdev->driver->name, sizeof(interface.name)); Should this even allow truncation? Perhaps it'd be better to fail in case of an overrun? Cheers,
On 2020/6/4 上午11:39, Herbert Xu wrote: > On Thu, Jun 04, 2020 at 11:32:04AM +0800, Zhangfei Gao wrote: >> Use strlcpy to fix the warning >> warning: 'strncpy' specified bound 64 equals destination size >> [-Wstringop-truncation] >> >> Reported-by: kernel test robot <lkp@intel.com> >> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> >> --- >> drivers/crypto/hisilicon/qm.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/crypto/hisilicon/qm.c b/drivers/crypto/hisilicon/qm.c >> index f795fb5..224f3e2 100644 >> --- a/drivers/crypto/hisilicon/qm.c >> +++ b/drivers/crypto/hisilicon/qm.c >> @@ -1574,7 +1574,7 @@ static int qm_alloc_uacce(struct hisi_qm *qm) >> .ops = &uacce_qm_ops, >> }; >> >> - strncpy(interface.name, pdev->driver->name, sizeof(interface.name)); >> + strlcpy(interface.name, pdev->driver->name, sizeof(interface.name)); > Should this even allow truncation? Perhaps it'd be better to fail > in case of an overrun? I think we do not need consider overrun, since it at most copy size-1 bytes to dest. From the manual: strlcpy() This function is similar to strncpy(), but it copies at most size-1 bytes to dest, always adds a terminating null byte, And simple tested with smaller SIZE of interface.name, only SIZE-1 is copied, so it is safe. -#define UACCE_MAX_NAME_SIZE 64 +#define UACCE_MAX_NAME_SIZE 4 Thanks
On Thu, Jun 04, 2020 at 02:10:37PM +0800, Zhangfei Gao wrote: > > > Should this even allow truncation? Perhaps it'd be better to fail > > in case of an overrun? > I think we do not need consider overrun, since it at most copy size-1 bytes > to dest. > From the manual: strlcpy() > This function is similar to strncpy(), but it copies at most > size-1 bytes to dest, always adds a terminating null > byte, > And simple tested with smaller SIZE of interface.name, only SIZE-1 is > copied, so it is safe. > -#define UACCE_MAX_NAME_SIZE 64 > +#define UACCE_MAX_NAME_SIZE 4 That's not what I meant. As it is if you do exceed the limit the name is silently truncated. Wouldn't it be better to fail the allocation instead? Cheers,
On 2020/6/4 下午2:18, Herbert Xu wrote: > On Thu, Jun 04, 2020 at 02:10:37PM +0800, Zhangfei Gao wrote: >>> Should this even allow truncation? Perhaps it'd be better to fail >>> in case of an overrun? >> I think we do not need consider overrun, since it at most copy size-1 bytes >> to dest. >> From the manual: strlcpy() >> This function is similar to strncpy(), but it copies at most >> size-1 bytes to dest, always adds a terminating null >> byte, >> And simple tested with smaller SIZE of interface.name, only SIZE-1 is >> copied, so it is safe. >> -#define UACCE_MAX_NAME_SIZE 64 >> +#define UACCE_MAX_NAME_SIZE 4 > That's not what I meant. As it is if you do exceed the limit the > name is silently truncated. Wouldn't it be better to fail the > allocation instead? I think it is fine. 1. Currently the name size is 64, bigger enough. Simply grep in driver name, 64 should be enough. We can make it larger when there is a request. 2. it does not matter what the name is, since it is just an interface. cat /sys/class/uacce/hisi_zip-0/flags cat /sys/class/uacce/his-0/flags should be both fine to app only they can be distinguished. 3. It maybe a hard restriction to fail just because of a long name. What do you think. Thanks
On Thu, Jun 04, 2020 at 02:44:16PM +0800, Zhangfei Gao wrote: > > I think it is fine. > 1. Currently the name size is 64, bigger enough. > Simply grep in driver name, 64 should be enough. > We can make it larger when there is a request. > 2. it does not matter what the name is, since it is just an interface. > cat /sys/class/uacce/hisi_zip-0/flags > cat /sys/class/uacce/his-0/flags > should be both fine to app only they can be distinguished. > 3. It maybe a hard restriction to fail just because of a long name. I think we should err on the side of caution. IOW, unless you know that you need it to succeed when it exceeds the limit, then you should just make it fail. Thanks,
On 2020/6/4 14:50, Herbert Xu wrote: > On Thu, Jun 04, 2020 at 02:44:16PM +0800, Zhangfei Gao wrote: >> >> I think it is fine. >> 1. Currently the name size is 64, bigger enough. >> Simply grep in driver name, 64 should be enough. >> We can make it larger when there is a request. >> 2. it does not matter what the name is, since it is just an interface. >> cat /sys/class/uacce/hisi_zip-0/flags >> cat /sys/class/uacce/his-0/flags >> should be both fine to app only they can be distinguished. >> 3. It maybe a hard restriction to fail just because of a long name. > > I think we should err on the side of caution. IOW, unless you > know that you need it to succeed when it exceeds the limit, then > you should just make it fail. Yes. We need make it fail to avoid silent truncation. > > Thanks, >
On 2020/6/4 下午2:50, Herbert Xu wrote: > On Thu, Jun 04, 2020 at 02:44:16PM +0800, Zhangfei Gao wrote: >> I think it is fine. >> 1. Currently the name size is 64, bigger enough. >> Simply grep in driver name, 64 should be enough. >> We can make it larger when there is a request. >> 2. it does not matter what the name is, since it is just an interface. >> cat /sys/class/uacce/hisi_zip-0/flags >> cat /sys/class/uacce/his-0/flags >> should be both fine to app only they can be distinguished. >> 3. It maybe a hard restriction to fail just because of a long name. > I think we should err on the side of caution. IOW, unless you > know that you need it to succeed when it exceeds the limit, then > you should just make it fail. Thanks Herbert Will add a check after the copy. strlcpy(interface.name, pdev->driver->name, sizeof(interface.name)); if (strlen(pdev->driver->name) != strlen(interface.name)) return -EINVAL; Will resend the fix after rc1 is open. Thanks
On Fri, Jun 05, 2020 at 05:34:32PM +0800, Zhangfei Gao wrote: > Will add a check after the copy. > > strlcpy(interface.name, pdev->driver->name, sizeof(interface.name)); > if (strlen(pdev->driver->name) != strlen(interface.name)) > return -EINVAL; You don't need to do strlen. The function strlcpy returns the length of the source string. Better yet use strscpy which will even return an error for you. Cheers,
On 2020/6/5 下午8:17, Herbert Xu wrote: > On Fri, Jun 05, 2020 at 05:34:32PM +0800, Zhangfei Gao wrote: >> Will add a check after the copy. >> >> strlcpy(interface.name, pdev->driver->name, sizeof(interface.name)); >> if (strlen(pdev->driver->name) != strlen(interface.name)) >> return -EINVAL; > You don't need to do strlen. The function strlcpy returns the > length of the source string. > > Better yet use strscpy which will even return an error for you. > > Yes, good idea, we can use strscpy. + int ret; - strncpy(interface.name, pdev->driver->name, sizeof(interface.name)); + ret = strscpy(interface.name, pdev->driver->name, sizeof(interface.name)); + if (ret < 0) + return ret; Will resend later, thanks Herbert.
On Fri, Jun 05, 2020 at 11:26:20PM +0800, Zhangfei Gao wrote: > > > On 2020/6/5 下午8:17, Herbert Xu wrote: > > On Fri, Jun 05, 2020 at 05:34:32PM +0800, Zhangfei Gao wrote: > > > Will add a check after the copy. > > > > > > strlcpy(interface.name, pdev->driver->name, sizeof(interface.name)); > > > if (strlen(pdev->driver->name) != strlen(interface.name)) > > > return -EINVAL; > > You don't need to do strlen. The function strlcpy returns the > > length of the source string. > > > > Better yet use strscpy which will even return an error for you. > > > > > Yes, good idea, we can use strscpy. > > + int ret; > > - strncpy(interface.name, pdev->driver->name, sizeof(interface.name)); > + ret = strscpy(interface.name, pdev->driver->name, > sizeof(interface.name)); > + if (ret < 0) > + return ret; You might want to use -ENAMETOOLONG instead of the strscpy return value of -E2BIG.
On 2020/6/5 下午11:49, Eric Biggers wrote: > On Fri, Jun 05, 2020 at 11:26:20PM +0800, Zhangfei Gao wrote: >> >> On 2020/6/5 下午8:17, Herbert Xu wrote: >>> On Fri, Jun 05, 2020 at 05:34:32PM +0800, Zhangfei Gao wrote: >>>> Will add a check after the copy. >>>> >>>> strlcpy(interface.name, pdev->driver->name, sizeof(interface.name)); >>>> if (strlen(pdev->driver->name) != strlen(interface.name)) >>>> return -EINVAL; >>> You don't need to do strlen. The function strlcpy returns the >>> length of the source string. >>> >>> Better yet use strscpy which will even return an error for you. >>> >>> >> Yes, good idea, we can use strscpy. >> >> + int ret; >> >> - strncpy(interface.name, pdev->driver->name, sizeof(interface.name)); >> + ret = strscpy(interface.name, pdev->driver->name, >> sizeof(interface.name)); >> + if (ret < 0) >> + return ret; > You might want to use -ENAMETOOLONG instead of the strscpy return value of > -E2BIG. Yes, make sense, thanks Eric
From: Herbert Xu > Sent: 05 June 2020 13:17 ... > Better yet use strscpy which will even return an error for you. It really ought to return the buffer length on truncation. Then you can loop: while(...) buf += strxxxcpy(buf, src, buf_end - buf); and only check right at the end. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Sun, Jun 07, 2020 at 01:03:45PM +0000, David Laight wrote: > From: Herbert Xu > > Sent: 05 June 2020 13:17 > ... > > Better yet use strscpy which will even return an error for you. > > It really ought to return the buffer length on truncation. > Then you can loop: > while(...) > buf += strxxxcpy(buf, src, buf_end - buf); > and only check right at the end. > > David scnprintf() can be used for that. But that doesn't seem relevant to this patch. - Eric
diff --git a/drivers/crypto/hisilicon/qm.c b/drivers/crypto/hisilicon/qm.c index f795fb5..224f3e2 100644 --- a/drivers/crypto/hisilicon/qm.c +++ b/drivers/crypto/hisilicon/qm.c @@ -1574,7 +1574,7 @@ static int qm_alloc_uacce(struct hisi_qm *qm) .ops = &uacce_qm_ops, }; - strncpy(interface.name, pdev->driver->name, sizeof(interface.name)); + strlcpy(interface.name, pdev->driver->name, sizeof(interface.name)); uacce = uacce_alloc(&pdev->dev, &interface); if (IS_ERR(uacce))
Use strlcpy to fix the warning warning: 'strncpy' specified bound 64 equals destination size [-Wstringop-truncation] Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> --- drivers/crypto/hisilicon/qm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)