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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sun, 9 Aug 2020 11:19:31 +0800 From: "luobin (L)" <luobin9@...wei.com> To: Kees Cook <keescook@...omium.org> CC: David Miller <davem@...emloft.net>, <David.Laight@...LAB.COM>, <linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>, <luoxianjun@...wei.com>, <yin.yinshi@...wei.com>, <cloud.wangxiaoyun@...wei.com>, <chiqijun@...wei.com> Subject: Re: [PATCH net-next v1] hinic: fix strncpy output truncated compile warnings 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@...wei.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!
Powered by blists - more mailing lists