netdev - Re: [PATCH net] net: hns3: fix strscpy causing content truncation issue

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] [thread-next>] [day] [month] [year] [list]

Date: Thu, 10 Aug 2023 10:22:47 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Jijie Shao <shaojijie@...wei.com>, Kees Cook <keescook@...omium.org>
Cc: Leon Romanovsky <leon@...nel.org>, <yisen.zhuang@...wei.com>,
 <salil.mehta@...wei.com>, <davem@...emloft.net>, <edumazet@...gle.com>,
 <pabeni@...hat.com>, <shenjian15@...wei.com>, <wangjie125@...wei.com>,
 <liuyonglong@...wei.com>, <chenhao418@...wei.com>,
 <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <stable@...r.kernel.org>
Subject: Re: [PATCH net] net: hns3: fix strscpy causing content truncation
 issue

On Thu, 10 Aug 2023 15:45:50 +0800 Jijie Shao wrote:
> on 2023/8/9 15:03, Leon Romanovsky wrote:
> > On Wed, Aug 09, 2023 at 10:09:02AM +0800, Jijie Shao wrote:  
> >> From: Hao Chen <chenhao418@...wei.com>
> >>
> >> hns3_dbg_fill_content()/hclge_dbg_fill_content() is aim to integrate some
> >> items to a string for content, and we add '\n' and '\0' in the last
> >> two bytes of content.
> >>
> >> strscpy() will add '\0' in the last byte of destination buffer(one of
> >> items), it result in finishing content print ahead of schedule and some
> >> dump content truncation.
> >>
> >> One Error log shows as below:
> >> cat mac_list/uc
> >> UC MAC_LIST:
> >>
> >> Expected:
> >> UC MAC_LIST:
> >> FUNC_ID  MAC_ADDR            STATE
> >> pf       00:2b:19:05:03:00   ACTIVE
> >>
> >> The destination buffer is length-bounded and not required to be
> >> NUL-terminated, so just change strscpy() to memcpy() to fix it.  
> > I think that you should change to strtomem() and not use plain memcpy().
> >
> > Thanks  
> 
> Hi:
> 
> We tried to replace memcpy with strtomem, but errors was reported during 
> compilation:
> /kernel/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c: In 
> function ‘hclge_dbg_fill_content.part.0’:
> /kernel/include/linux/compiler_types.h:397:38: error: call to 
> ‘__compiletime_assert_519’ declared with attribute error: BUILD_BUG_ON 
> failed: !__builtin_constant_p(_dest_len) || _dest_len == (size_t)-1
>    397 |  _compiletime_assert(condition, msg, __compiletime_assert_, 
> __COUNTER__)
>        |                                      ^
> /kernel/include/linux/compiler_types.h:378:4: note: in definition of 
> macro ‘__compiletime_assert’
>    378 |    prefix ## suffix();    \
>        |    ^~~~~~
> /kernel/include/linux/compiler_types.h:397:2: note: in expansion of 
> macro ‘_compiletime_assert’
>    397 |  _compiletime_assert(condition, msg, __compiletime_assert_, 
> __COUNTER__)
>        |  ^~~~~~~~~~~~~~~~~~~
> /kernel/include/linux/build_bug.h:39:37: note: in expansion of macro 
> ‘compiletime_assert’
>     39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), 
> msg)
>        |                                     ^~~~~~~~~~~~~~~~~~
> /kernel/include/linux/build_bug.h:50:2: note: in expansion of macro 
> ‘BUILD_BUG_ON_MSG’
>     50 |  BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
>        |  ^~~~~~~~~~~~~~~~
> /kernel/include/linux/string.h:302:2: note: in expansion of macro 
> ‘BUILD_BUG_ON’
>    302 |  BUILD_BUG_ON(!__builtin_constant_p(_dest_len) ||  \
>        |  ^~~~~~~~~~~~
> /kernel/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c:115:4: 
> note: in expansion of macro ‘strtomem’
>    115 |    strtomem(pos, result[i]);
>        |    ^~~~~~~~
> 
> In the strtomem macro, __builtin_object_size is used to calculate the 
> _dest_len.
> We tried to print the _dest_len directly, and the result was -1.
> How can we solve this?

Let's add Kees in case he has a immediate recommendation on use of
strtomem() vs memcpy() for this case..