* [PATCH net-next v1] hinic: fix strncpy output truncated compile warnings @ 2020-08-07 2:09 Luo bin 2020-08-07 9:32 ` David Laight 0 siblings, 1 reply; 10+ messages in thread From: Luo bin @ 2020-08-07 2:09 UTC (permalink / raw) To: davem Cc: linux-kernel, netdev, luoxianjun, yin.yinshi, cloud.wangxiaoyun, chiqijun fix the compile warnings of 'strncpy' output truncated before terminating nul copying N bytes from a string of the same length Signed-off-by: Luo bin <luobin9@huawei.com> Reported-by: kernel test robot <lkp@intel.com> --- V0~V1: - use the strlen()+1 pattern consistently drivers/net/ethernet/huawei/hinic/hinic_devlink.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c index c6adc776f3c8..1ec88ebf81d6 100644 --- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c +++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c @@ -342,9 +342,9 @@ static int chip_fault_show(struct devlink_fmsg *fmsg, level = event->event.chip.err_level; if (level < FAULT_LEVEL_MAX) - strncpy(level_str, fault_level[level], strlen(fault_level[level])); + strncpy(level_str, fault_level[level], strlen(fault_level[level]) + 1); else - strncpy(level_str, "Unknown", strlen("Unknown")); + strncpy(level_str, "Unknown", strlen("Unknown") + 1); if (level == FAULT_LEVEL_SERIOUS_FLR) { err = devlink_fmsg_u32_pair_put(fmsg, "Function level err func_id", @@ -388,9 +388,9 @@ static int fault_report_show(struct devlink_fmsg *fmsg, int err; if (event->type < FAULT_TYPE_MAX) - strncpy(type_str, fault_type[event->type], strlen(fault_type[event->type])); + strncpy(type_str, fault_type[event->type], strlen(fault_type[event->type]) + 1); else - strncpy(type_str, "Unknown", strlen("Unknown")); + strncpy(type_str, "Unknown", strlen("Unknown") + 1); err = devlink_fmsg_string_pair_put(fmsg, "Fault type", type_str); if (err) -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [PATCH net-next v1] hinic: fix strncpy output truncated compile warnings 2020-08-07 2:09 [PATCH net-next v1] hinic: fix strncpy output truncated compile warnings Luo bin @ 2020-08-07 9:32 ` David Laight 2020-08-08 3:36 ` luobin (L) 0 siblings, 1 reply; 10+ messages in thread From: David Laight @ 2020-08-07 9:32 UTC (permalink / raw) To: 'Luo bin', davem@davemloft.net Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org, luoxianjun@huawei.com, yin.yinshi@huawei.com, cloud.wangxiaoyun@huawei.com, chiqijun@huawei.com From: Luo bin > Sent: 07 August 2020 03:09 > > fix the compile warnings of 'strncpy' output truncated before > terminating nul copying N bytes from a string of the same length > > Signed-off-by: Luo bin <luobin9@huawei.com> > Reported-by: kernel test robot <lkp@intel.com> > --- > V0~V1: > - use the strlen()+1 pattern consistently > > drivers/net/ethernet/huawei/hinic/hinic_devlink.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c > b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c > index c6adc776f3c8..1ec88ebf81d6 100644 > --- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c > +++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c > @@ -342,9 +342,9 @@ static int chip_fault_show(struct devlink_fmsg *fmsg, > > level = event->event.chip.err_level; > if (level < FAULT_LEVEL_MAX) > - strncpy(level_str, fault_level[level], strlen(fault_level[level])); > + strncpy(level_str, fault_level[level], strlen(fault_level[level]) + 1); Have you even considered what that code is actually doing? David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v1] hinic: fix strncpy output truncated compile warnings 2020-08-07 9:32 ` David Laight @ 2020-08-08 3:36 ` luobin (L) 2020-08-08 3:42 ` David Miller 2020-08-08 12:50 ` David Laight 0 siblings, 2 replies; 10+ messages in thread From: luobin (L) @ 2020-08-08 3:36 UTC (permalink / raw) To: David Laight, davem@davemloft.net Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org, luoxianjun@huawei.com, yin.yinshi@huawei.com, cloud.wangxiaoyun@huawei.com, chiqijun@huawei.com On 2020/8/7 17:32, David Laight wrote: > From: Luo bin >> Sent: 07 August 2020 03:09 >> >> fix the compile warnings of 'strncpy' output truncated before >> terminating nul copying N bytes from a string of the same length >> >> Signed-off-by: Luo bin <luobin9@huawei.com> >> Reported-by: kernel test robot <lkp@intel.com> >> --- >> V0~V1: >> - use the strlen()+1 pattern consistently >> >> drivers/net/ethernet/huawei/hinic/hinic_devlink.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >> b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >> index c6adc776f3c8..1ec88ebf81d6 100644 >> --- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >> +++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >> @@ -342,9 +342,9 @@ static int chip_fault_show(struct devlink_fmsg *fmsg, >> >> level = event->event.chip.err_level; >> if (level < FAULT_LEVEL_MAX) >> - strncpy(level_str, fault_level[level], strlen(fault_level[level])); >> + strncpy(level_str, fault_level[level], strlen(fault_level[level]) + 1); > > Have you even considered what that code is actually doing? > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) > > . > I'm sorry that I haven't got what you mean and I haven't found any defects in that code. Can you explain more to me? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v1] hinic: fix strncpy output truncated compile warnings 2020-08-08 3:36 ` luobin (L) @ 2020-08-08 3:42 ` David Miller 2020-08-08 6:44 ` Kees Cook 2020-08-09 2:59 ` luobin (L) 2020-08-08 12:50 ` David Laight 1 sibling, 2 replies; 10+ messages in thread From: David Miller @ 2020-08-08 3:42 UTC (permalink / raw) To: luobin9 Cc: David.Laight, linux-kernel, netdev, luoxianjun, yin.yinshi, cloud.wangxiaoyun, chiqijun From: "luobin (L)" <luobin9@huawei.com> Date: Sat, 8 Aug 2020 11:36:42 +0800 > On 2020/8/7 17:32, David Laight wrote: >>> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >>> b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >>> index c6adc776f3c8..1ec88ebf81d6 100644 >>> --- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >>> +++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >>> @@ -342,9 +342,9 @@ static int chip_fault_show(struct devlink_fmsg *fmsg, >>> >>> level = event->event.chip.err_level; >>> if (level < FAULT_LEVEL_MAX) >>> - strncpy(level_str, fault_level[level], strlen(fault_level[level])); >>> + strncpy(level_str, fault_level[level], strlen(fault_level[level]) + 1); >> >> Have you even considered what that code is actually doing? ... > I'm sorry that I haven't got what you mean and I haven't found any defects in that code. Can you explain more to me? David is trying to express the same thing I was trying to explain to you, you should use sizeof(level_str) as the third argument because the code is trying to make sure that the destination buffer is not overrun. If you use the strlen() of the source buffer, the strncpy() can still overflow the destination buffer. Now do you understand? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v1] hinic: fix strncpy output truncated compile warnings 2020-08-08 3:42 ` David Miller @ 2020-08-08 6:44 ` Kees Cook 2020-08-09 3:19 ` luobin (L) 2020-08-09 2:59 ` luobin (L) 1 sibling, 1 reply; 10+ messages in thread From: Kees Cook @ 2020-08-08 6:44 UTC (permalink / raw) To: luobin9 Cc: David Miller, David.Laight, linux-kernel, netdev, luoxianjun, yin.yinshi, cloud.wangxiaoyun, chiqijun On Fri, Aug 07, 2020 at 08:42:43PM -0700, David Miller wrote: > From: "luobin (L)" <luobin9@huawei.com> > Date: Sat, 8 Aug 2020 11:36:42 +0800 > > > On 2020/8/7 17:32, David Laight wrote: > >>> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c > >>> b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c > >>> index c6adc776f3c8..1ec88ebf81d6 100644 > >>> --- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c > >>> +++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c > >>> @@ -342,9 +342,9 @@ static int chip_fault_show(struct devlink_fmsg *fmsg, > >>> > >>> level = event->event.chip.err_level; > >>> if (level < FAULT_LEVEL_MAX) > >>> - strncpy(level_str, fault_level[level], strlen(fault_level[level])); > >>> + strncpy(level_str, fault_level[level], strlen(fault_level[level]) + 1); > >> > >> Have you even considered what that code is actually doing? > ... > > I'm sorry that I haven't got what you mean and I haven't found any defects in that code. Can you explain more to me? > > David is trying to express the same thing I was trying to explain to > you, you should use sizeof(level_str) as the third argument because > the code is trying to make sure that the destination buffer is not > overrun. > > If you use the strlen() of the source buffer, the strncpy() can still > overflow the destination buffer. > > Now do you understand? Agh, please never use strncpy() on NUL-terminated strings[1]. (You can see this ultimately gets passed down into devlink_fmsg_string_put() which expects NUL-terminated strings and does not require trailing NUL-padding (which if it did, should still never use strncpy(), but rather strscpy_pad()). But, as David Laight hints, none of this is needed. The entire buffer can be avoided (just point into the existing array of strings -- which should also be const). Add I see that one of the array sizes is wrong. Both use FAULT_TYPE_MAX, but one should be FAULT_LEVEL_MAX. And since "Unknown" can just be added to the array, do that and clamp the value since it's only used for finding the strings in the array. I would suggest this (totally untested): diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c index c6adc776f3c8..20bfb05896e5 100644 --- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c +++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c @@ -334,18 +334,12 @@ void hinic_devlink_unregister(struct hinic_devlink_priv *priv) static int chip_fault_show(struct devlink_fmsg *fmsg, struct hinic_fault_event *event) { - char fault_level[FAULT_TYPE_MAX][FAULT_SHOW_STR_LEN + 1] = { - "fatal", "reset", "flr", "general", "suggestion"}; - char level_str[FAULT_SHOW_STR_LEN + 1] = {0}; - u8 level; + const char * const level_str[FAULT_LEVEL_MAX + 1] = { + "fatal", "reset", "flr", "general", "suggestion", + [FAULT_LEVEL_MAX] = "Unknown"}; + u8 fault_level; int err; - level = event->event.chip.err_level; - if (level < FAULT_LEVEL_MAX) - strncpy(level_str, fault_level[level], strlen(fault_level[level])); - else - strncpy(level_str, "Unknown", strlen("Unknown")); - if (level == FAULT_LEVEL_SERIOUS_FLR) { err = devlink_fmsg_u32_pair_put(fmsg, "Function level err func_id", (u32)event->event.chip.func_id); @@ -361,7 +355,8 @@ static int chip_fault_show(struct devlink_fmsg *fmsg, if (err) return err; - err = devlink_fmsg_string_pair_put(fmsg, "err_level", level_str); + fault_level = clamp(event->event.chip.err_level, FAULT_LEVEL_MAX); + err = devlink_fmsg_string_pair_put(fmsg, "err_level", fault_str[fault_level]); if (err) return err; @@ -381,18 +376,15 @@ static int chip_fault_show(struct devlink_fmsg *fmsg, static int fault_report_show(struct devlink_fmsg *fmsg, struct hinic_fault_event *event) { - char fault_type[FAULT_TYPE_MAX][FAULT_SHOW_STR_LEN + 1] = { + const char * const type_str[FAULT_TYPE_MAX + 1] = { "chip", "ucode", "mem rd timeout", "mem wr timeout", - "reg rd timeout", "reg wr timeout", "phy fault"}; - char type_str[FAULT_SHOW_STR_LEN + 1] = {0}; + "reg rd timeout", "reg wr timeout", "phy fault", + [FAULT_TYPE_MAX] = "Unknown"}; + u8 fault_type; int err; - if (event->type < FAULT_TYPE_MAX) - strncpy(type_str, fault_type[event->type], strlen(fault_type[event->type])); - else - strncpy(type_str, "Unknown", strlen("Unknown")); - - err = devlink_fmsg_string_pair_put(fmsg, "Fault type", type_str); + fault_type = clamp(event->type, FAULT_TYPE_MAX); + err = devlink_fmsg_string_pair_put(fmsg, "Fault type", type_str[fault_type]); if (err) return err; -Kees [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings -- Kees Cook ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v1] hinic: fix strncpy output truncated compile warnings 2020-08-08 6:44 ` Kees Cook @ 2020-08-09 3:19 ` luobin (L) 2020-08-10 8:15 ` David Laight 0 siblings, 1 reply; 10+ messages in thread From: luobin (L) @ 2020-08-09 3:19 UTC (permalink / raw) To: Kees Cook Cc: David Miller, David.Laight, linux-kernel, netdev, luoxianjun, yin.yinshi, cloud.wangxiaoyun, chiqijun On 2020/8/8 14:44, Kees Cook wrote: > On Fri, Aug 07, 2020 at 08:42:43PM -0700, David Miller wrote: >> From: "luobin (L)" <luobin9@huawei.com> >> Date: Sat, 8 Aug 2020 11:36:42 +0800 >> >>> On 2020/8/7 17:32, David Laight wrote: >>>>> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >>>>> b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >>>>> index c6adc776f3c8..1ec88ebf81d6 100644 >>>>> --- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >>>>> +++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >>>>> @@ -342,9 +342,9 @@ static int chip_fault_show(struct devlink_fmsg *fmsg, >>>>> >>>>> level = event->event.chip.err_level; >>>>> if (level < FAULT_LEVEL_MAX) >>>>> - strncpy(level_str, fault_level[level], strlen(fault_level[level])); >>>>> + strncpy(level_str, fault_level[level], strlen(fault_level[level]) + 1); >>>> >>>> Have you even considered what that code is actually doing? >> ... >>> I'm sorry that I haven't got what you mean and I haven't found any defects in that code. Can you explain more to me? >> >> David is trying to express the same thing I was trying to explain to >> you, you should use sizeof(level_str) as the third argument because >> the code is trying to make sure that the destination buffer is not >> overrun. >> >> If you use the strlen() of the source buffer, the strncpy() can still >> overflow the destination buffer. >> >> Now do you understand? > > Agh, please never use strncpy() on NUL-terminated strings[1]. (You can > see this ultimately gets passed down into devlink_fmsg_string_put() > which expects NUL-terminated strings and does not require trailing > NUL-padding (which if it did, should still never use strncpy(), but > rather strscpy_pad()). > > But, as David Laight hints, none of this is needed. The entire buffer > can be avoided (just point into the existing array of strings -- which > should also be const). Add I see that one of the array sizes is wrong. > Both use FAULT_TYPE_MAX, but one should be FAULT_LEVEL_MAX. And since > "Unknown" can just be added to the array, do that and clamp the value > since it's only used for finding the strings in the array. > > I would suggest this (totally untested): > > diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c > index c6adc776f3c8..20bfb05896e5 100644 > --- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c > +++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c > @@ -334,18 +334,12 @@ void hinic_devlink_unregister(struct hinic_devlink_priv *priv) > static int chip_fault_show(struct devlink_fmsg *fmsg, > struct hinic_fault_event *event) > { > - char fault_level[FAULT_TYPE_MAX][FAULT_SHOW_STR_LEN + 1] = { > - "fatal", "reset", "flr", "general", "suggestion"}; > - char level_str[FAULT_SHOW_STR_LEN + 1] = {0}; > - u8 level; > + const char * const level_str[FAULT_LEVEL_MAX + 1] = { > + "fatal", "reset", "flr", "general", "suggestion", > + [FAULT_LEVEL_MAX] = "Unknown"}; > + u8 fault_level; > int err; > > - level = event->event.chip.err_level; > - if (level < FAULT_LEVEL_MAX) > - strncpy(level_str, fault_level[level], strlen(fault_level[level])); > - else > - strncpy(level_str, "Unknown", strlen("Unknown")); > - > if (level == FAULT_LEVEL_SERIOUS_FLR) { > err = devlink_fmsg_u32_pair_put(fmsg, "Function level err func_id", > (u32)event->event.chip.func_id); > @@ -361,7 +355,8 @@ static int chip_fault_show(struct devlink_fmsg *fmsg, > if (err) > return err; > > - err = devlink_fmsg_string_pair_put(fmsg, "err_level", level_str); > + fault_level = clamp(event->event.chip.err_level, FAULT_LEVEL_MAX); > + err = devlink_fmsg_string_pair_put(fmsg, "err_level", fault_str[fault_level]); > if (err) > return err; > > @@ -381,18 +376,15 @@ static int chip_fault_show(struct devlink_fmsg *fmsg, > static int fault_report_show(struct devlink_fmsg *fmsg, > struct hinic_fault_event *event) > { > - char fault_type[FAULT_TYPE_MAX][FAULT_SHOW_STR_LEN + 1] = { > + const char * const type_str[FAULT_TYPE_MAX + 1] = { > "chip", "ucode", "mem rd timeout", "mem wr timeout", > - "reg rd timeout", "reg wr timeout", "phy fault"}; > - char type_str[FAULT_SHOW_STR_LEN + 1] = {0}; > + "reg rd timeout", "reg wr timeout", "phy fault", > + [FAULT_TYPE_MAX] = "Unknown"}; > + u8 fault_type; > int err; > > - if (event->type < FAULT_TYPE_MAX) > - strncpy(type_str, fault_type[event->type], strlen(fault_type[event->type])); > - else > - strncpy(type_str, "Unknown", strlen("Unknown")); > - > - err = devlink_fmsg_string_pair_put(fmsg, "Fault type", type_str); > + fault_type = clamp(event->type, FAULT_TYPE_MAX); > + err = devlink_fmsg_string_pair_put(fmsg, "Fault type", type_str[fault_type]); > if (err) > return err; > > > > -Kees > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > Thanks for your explanation and review. I haven't realized using strncpy() on NUL-terminated strings is deprecated and just trying to avoid the compile warnings. The website you provide helps me a lot. Thank you very much! ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH net-next v1] hinic: fix strncpy output truncated compile warnings 2020-08-09 3:19 ` luobin (L) @ 2020-08-10 8:15 ` David Laight 0 siblings, 0 replies; 10+ messages in thread From: David Laight @ 2020-08-10 8:15 UTC (permalink / raw) To: 'luobin (L)', Kees Cook Cc: David Miller, linux-kernel@vger.kernel.org, netdev@vger.kernel.org, luoxianjun@huawei.com, yin.yinshi@huawei.com, cloud.wangxiaoyun@huawei.com, chiqijun@huawei.com > Thanks for your explanation and review. I haven't realized using strncpy() on NUL-terminated strings > is deprecated > and just trying to avoid the compile warnings. The website you provide helps me a lot. Thank you very > much! Never try to remove compile-time warnings without understanding what the code it doing. The basic problem is that strncpy() almost [1] never does what you want. It really expects it's input string to be '\0' terminated but doesn't guarantee the output will be, and also (typically) wastes cpu cycles zero filling the output buffer. Someone then defined strscpy() as an alternative, it guarantees to '\0' the output and doesn't zero fill - which can be an issue. However strscpy() has it's own problems, the return value is defined to be the length of the input string - which absolutely requires it be '\0' terminated. With 'unknown' input this can page fault! [1] This fragment looked wrong, but was right! strncpy(dest, src, sizeof src); Naive conversion to remove the strncpy() broke it. In fact 'dest' was 1 byte longer than 'src' and already zero filled, 'src' might not have been '\0' terminated. It is about the only time strncpy() is what you want! David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v1] hinic: fix strncpy output truncated compile warnings 2020-08-08 3:42 ` David Miller 2020-08-08 6:44 ` Kees Cook @ 2020-08-09 2:59 ` luobin (L) 1 sibling, 0 replies; 10+ messages in thread From: luobin (L) @ 2020-08-09 2:59 UTC (permalink / raw) To: David Miller Cc: David.Laight, linux-kernel, netdev, luoxianjun, yin.yinshi, cloud.wangxiaoyun, chiqijun On 2020/8/8 11:42, David Miller wrote: > From: "luobin (L)" <luobin9@huawei.com> > Date: Sat, 8 Aug 2020 11:36:42 +0800 > >> On 2020/8/7 17:32, David Laight wrote: >>>> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >>>> b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >>>> index c6adc776f3c8..1ec88ebf81d6 100644 >>>> --- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >>>> +++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >>>> @@ -342,9 +342,9 @@ static int chip_fault_show(struct devlink_fmsg *fmsg, >>>> >>>> level = event->event.chip.err_level; >>>> if (level < FAULT_LEVEL_MAX) >>>> - strncpy(level_str, fault_level[level], strlen(fault_level[level])); >>>> + strncpy(level_str, fault_level[level], strlen(fault_level[level]) + 1); >>> >>> Have you even considered what that code is actually doing? > ... >> I'm sorry that I haven't got what you mean and I haven't found any defects in that code. Can you explain more to me? > > David is trying to express the same thing I was trying to explain to > you, you should use sizeof(level_str) as the third argument because > the code is trying to make sure that the destination buffer is not > overrun. > > If you use the strlen() of the source buffer, the strncpy() can still > overflow the destination buffer. > > Now do you understand? > . > Thanks for your explanation. I explained that why I didn't use sizeof(level_str) as the third argument in my previous reply e-mail to you. Because using sizeof(level_str) as the third argument will still cause the following compile warning: In function ‘strncpy’, inlined from ‘chip_fault_show’ at drivers/net/ethernet/huawei/hinic/hinic_devlink.c:345:3: ./include/linux/string.h:297:30: warning: ‘__builtin_strncpy’ specified bound 17 equals destination size [-Wstringop-truncation] 297 | #define __underlying_strncpy __builtin_strncpy Now I know that using strncpy() on NUL-terminated strings is deprecated as Kees Cook points out and actually there is no need to use it in my code. ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH net-next v1] hinic: fix strncpy output truncated compile warnings 2020-08-08 3:36 ` luobin (L) 2020-08-08 3:42 ` David Miller @ 2020-08-08 12:50 ` David Laight 2020-08-09 3:35 ` luobin (L) 1 sibling, 1 reply; 10+ messages in thread From: David Laight @ 2020-08-08 12:50 UTC (permalink / raw) To: 'luobin (L)', davem@davemloft.net Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org, luoxianjun@huawei.com, yin.yinshi@huawei.com, cloud.wangxiaoyun@huawei.com, chiqijun@huawei.com From: luobin (L) > Sent: 08 August 2020 04:37 > > On 2020/8/7 17:32, David Laight wrote: > > From: Luo bin > >> Sent: 07 August 2020 03:09 > >> > >> fix the compile warnings of 'strncpy' output truncated before > >> terminating nul copying N bytes from a string of the same length > >> > >> Signed-off-by: Luo bin <luobin9@huawei.com> > >> Reported-by: kernel test robot <lkp@intel.com> > >> --- > >> V0~V1: > >> - use the strlen()+1 pattern consistently > >> > >> drivers/net/ethernet/huawei/hinic/hinic_devlink.c | 8 ++++---- > >> 1 file changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c > >> b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c > >> index c6adc776f3c8..1ec88ebf81d6 100644 > >> --- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c > >> +++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c > >> @@ -342,9 +342,9 @@ static int chip_fault_show(struct devlink_fmsg *fmsg, > >> > >> level = event->event.chip.err_level; > >> if (level < FAULT_LEVEL_MAX) > >> - strncpy(level_str, fault_level[level], strlen(fault_level[level])); > >> + strncpy(level_str, fault_level[level], strlen(fault_level[level]) + 1); > > > > Have you even considered what that code is actually doing? > > > > David > > I'm sorry that I haven't got what you mean and I haven't found any defects in that code. Can you > explain more to me? If you can't see it you probably shouldn't be submitting patches.... Consider what happens when the string is long. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v1] hinic: fix strncpy output truncated compile warnings 2020-08-08 12:50 ` David Laight @ 2020-08-09 3:35 ` luobin (L) 0 siblings, 0 replies; 10+ messages in thread From: luobin (L) @ 2020-08-09 3:35 UTC (permalink / raw) To: David Laight, davem@davemloft.net Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org, luoxianjun@huawei.com, yin.yinshi@huawei.com, cloud.wangxiaoyun@huawei.com, chiqijun@huawei.com On 2020/8/8 20:50, David Laight wrote: > From: luobin (L) >> Sent: 08 August 2020 04:37 >> >> On 2020/8/7 17:32, David Laight wrote: >>> From: Luo bin >>>> Sent: 07 August 2020 03:09 >>>> >>>> fix the compile warnings of 'strncpy' output truncated before >>>> terminating nul copying N bytes from a string of the same length >>>> >>>> Signed-off-by: Luo bin <luobin9@huawei.com> >>>> Reported-by: kernel test robot <lkp@intel.com> >>>> --- >>>> V0~V1: >>>> - use the strlen()+1 pattern consistently >>>> >>>> drivers/net/ethernet/huawei/hinic/hinic_devlink.c | 8 ++++---- >>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >>>> b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >>>> index c6adc776f3c8..1ec88ebf81d6 100644 >>>> --- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >>>> +++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >>>> @@ -342,9 +342,9 @@ static int chip_fault_show(struct devlink_fmsg *fmsg, >>>> >>>> level = event->event.chip.err_level; >>>> if (level < FAULT_LEVEL_MAX) >>>> - strncpy(level_str, fault_level[level], strlen(fault_level[level])); >>>> + strncpy(level_str, fault_level[level], strlen(fault_level[level]) + 1); >>> >>> Have you even considered what that code is actually doing? >>> >>> David >> >> I'm sorry that I haven't got what you mean and I haven't found any defects in that code. Can you >> explain more to me? > > If you can't see it you probably shouldn't be submitting patches.... > > Consider what happens when the string is long. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) > Thanks for your explanation and review. The fault_level[level] is a fixed and NUL-terminated character string in that code and its length is smaller than the dest buffer size so I think using strlen(fault_level[level]) + 1 will not overflow the destination buffer. But using strncpy() on NUL-terminated strings is dangerous indeed and there is totally no need to use it in that code as Kees points out. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-08-10 8:15 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-08-07 2:09 [PATCH net-next v1] hinic: fix strncpy output truncated compile warnings Luo bin 2020-08-07 9:32 ` David Laight 2020-08-08 3:36 ` luobin (L) 2020-08-08 3:42 ` David Miller 2020-08-08 6:44 ` Kees Cook 2020-08-09 3:19 ` luobin (L) 2020-08-10 8:15 ` David Laight 2020-08-09 2:59 ` luobin (L) 2020-08-08 12:50 ` David Laight 2020-08-09 3:35 ` luobin (L)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).