diff mbox series

[net] net: hns3: fix strscpy causing content truncation issue

Message ID 20230809020902.1941471-1-shaojijie@huawei.com (mailing list archive)
State Accepted
Commit 5e3d20617b055e725e785e0058426368269949f3
Delegated to: Netdev Maintainers
Headers show
Series [net] net: hns3: fix strscpy causing content truncation issue | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1328 this patch: 1328
netdev/cc_maintainers fail 1 blamed authors not CCed: lanhao@huawei.com; 2 maintainers not CCed: lanhao@huawei.com huangguangbin2@huawei.com
netdev/build_clang success Errors and warnings before: 1351 this patch: 1351
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 1351 this patch: 1351
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 22 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jijie Shao Aug. 9, 2023, 2:09 a.m. UTC
From: Hao Chen <chenhao418@huawei.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.

Fixes: 1cf3d5567f27 ("net: hns3: fix strncpy() not using dest-buf length as length issue")
Signed-off-by: Hao Chen <chenhao418@huawei.com>
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c         | 4 ++--
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

Comments

Leon Romanovsky Aug. 9, 2023, 7:03 a.m. UTC | #1
On Wed, Aug 09, 2023 at 10:09:02AM +0800, Jijie Shao wrote:
> From: Hao Chen <chenhao418@huawei.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
Jijie Shao Aug. 10, 2023, 7:45 a.m. UTC | #2
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@huawei.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?

Regards
Jijie Shao
Jakub Kicinski Aug. 10, 2023, 5:22 p.m. UTC | #3
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@huawei.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..
Kees Cook Aug. 10, 2023, 6:23 p.m. UTC | #4
On Thu, Aug 10, 2023 at 10:22:47AM -0700, Jakub Kicinski wrote:
> 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@huawei.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..

tldr: use memcpy() instead of strscpy().


Okay, I went to go read up on the history here. For my own notes, here's
the original code, prior to 1cf3d5567f27 ("net: hns3: fix strncpy()
not using dest-buf length as length issue"):

static void hns3_dbg_fill_content(char *content, u16 len,
				  const struct hns3_dbg_item *items,
				  const char **result, u16 size)
{
	char *pos = content;
	u16 i;

	memset(content, ' ', len);
	for (i = 0; i < size; i++) {
		if (result)
			strncpy(pos, result[i], strlen(result[i]));
		else
			strncpy(pos, items[i].name, strlen(items[i].name));

		pos += strlen(items[i].name) + items[i].interval;
	}

	*pos++ = '\n';
	*pos++ = '\0';
}

The warning to be fixed was:

hclge_debugfs.c:90:25: warning: 'strncpy' output truncated before terminating nul copying as many bytes from a string as its length [-Wstringop-truncation]

There are a few extra checks added in 1cf3d5567f27, but I'm more curious
about this original code's intent. It seems very confusing to me.

Firstly, why is "pos" updated based on "strlen(items[i].name)" even when
"result[i]" is used? Secondly, why is "interval" used? (These concerns
are mostly addressed in 1cf3d5567f27.)

I guess I'd just like to take a step back and ask, "What is this
function trying to do?" It seems to be building a series of strings in a
" "-padding buffer, and it intends that the buffer be newline and %NUL
terminated.

It looks very much like it wants to _avoid_ adding %NUL termination when
doing copies, which is why it's using strncpy with a length argument of
the source string length: it's _forcing_ the copy to not be terminated.
This is just memcpy.

strtomem() is designed for buffer sizes that can be known at compile
time, so it's not useful here (as was found), since a string is being
built up and uses a moving pointer.

I think the correct fix is to use memcpy() instead of strscpy(). No
%NUL-truncation is desired, the sizes are already determined and bounds
checked. (And the latter is what likely silenced the compiler warning.)

-Kees
Jakub Kicinski Aug. 10, 2023, 6:47 p.m. UTC | #5
On Thu, 10 Aug 2023 11:23:46 -0700 Kees Cook wrote:
> tldr: use memcpy() instead of strscpy().
> 
> 
> Okay, I went to go read up on the history here. For my own notes, here's
> the original code, prior to 1cf3d5567f27 ("net: hns3: fix strncpy()
> not using dest-buf length as length issue"):
> 
> static void hns3_dbg_fill_content(char *content, u16 len,
> 				  const struct hns3_dbg_item *items,
> 				  const char **result, u16 size)
> {
> 	char *pos = content;
> 	u16 i;
> 
> 	memset(content, ' ', len);
> 	for (i = 0; i < size; i++) {
> 		if (result)
> 			strncpy(pos, result[i], strlen(result[i]));
> 		else
> 			strncpy(pos, items[i].name, strlen(items[i].name));
> 
> 		pos += strlen(items[i].name) + items[i].interval;
> 	}
> 
> 	*pos++ = '\n';
> 	*pos++ = '\0';
> }
> 
> The warning to be fixed was:
> 
> hclge_debugfs.c:90:25: warning: 'strncpy' output truncated before terminating nul copying as many bytes from a string as its length [-Wstringop-truncation]
> 
> There are a few extra checks added in 1cf3d5567f27, but I'm more curious
> about this original code's intent. It seems very confusing to me.
> 
> Firstly, why is "pos" updated based on "strlen(items[i].name)" even when
> "result[i]" is used? Secondly, why is "interval" used? (These concerns
> are mostly addressed in 1cf3d5567f27.)
> 
> I guess I'd just like to take a step back and ask, "What is this
> function trying to do?" It seems to be building a series of strings in a
> " "-padding buffer, and it intends that the buffer be newline and %NUL
> terminated.
> 
> It looks very much like it wants to _avoid_ adding %NUL termination when
> doing copies, which is why it's using strncpy with a length argument of
> the source string length: it's _forcing_ the copy to not be terminated.
> This is just memcpy.
> 
> strtomem() is designed for buffer sizes that can be known at compile
> time, so it's not useful here (as was found), since a string is being
> built up and uses a moving pointer.
> 
> I think the correct fix is to use memcpy() instead of strscpy(). No
> %NUL-truncation is desired, the sizes are already determined and bounds
> checked. (And the latter is what likely silenced the compiler warning.)

Got it, thanks!
patchwork-bot+netdevbpf@kernel.org Aug. 10, 2023, 6:50 p.m. UTC | #6
Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 9 Aug 2023 10:09:02 +0800 you wrote:
> From: Hao Chen <chenhao418@huawei.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.
> 
> [...]

Here is the summary with links:
  - [net] net: hns3: fix strscpy causing content truncation issue
    https://git.kernel.org/netdev/net/c/5e3d20617b05

You are awesome, thank you!
Jijie Shao Aug. 11, 2023, 2:28 a.m. UTC | #7
on 2023/8/11 2:23, Kees Cook wrote:
>> Let's add Kees in case he has a immediate recommendation on use of
>> strtomem() vs memcpy() for this case..
> tldr: use memcpy() instead of strscpy().
>
>
> Okay, I went to go read up on the history here. For my own notes, here's
> the original code, prior to 1cf3d5567f27 ("net: hns3: fix strncpy()
> not using dest-buf length as length issue"):
>
> static void hns3_dbg_fill_content(char *content, u16 len,
> 				  const struct hns3_dbg_item *items,
> 				  const char **result, u16 size)
> {
> 	char *pos = content;
> 	u16 i;
>
> 	memset(content, ' ', len);
> 	for (i = 0; i < size; i++) {
> 		if (result)
> 			strncpy(pos, result[i], strlen(result[i]));
> 		else
> 			strncpy(pos, items[i].name, strlen(items[i].name));
>
> 		pos += strlen(items[i].name) + items[i].interval;
> 	}
>
> 	*pos++ = '\n';
> 	*pos++ = '\0';
> }
>
> The warning to be fixed was:
>
> hclge_debugfs.c:90:25: warning: 'strncpy' output truncated before terminating nul copying as many bytes from a string as its length [-Wstringop-truncation]
>
> There are a few extra checks added in 1cf3d5567f27, but I'm more curious
> about this original code's intent. It seems very confusing to me.
>
> Firstly, why is "pos" updated based on "strlen(items[i].name)" even when
> "result[i]" is used? Secondly, why is "interval" used? (These concerns
> are mostly addressed in 1cf3d5567f27.)
>
> I guess I'd just like to take a step back and ask, "What is this
> function trying to do?" It seems to be building a series of strings in a
> " "-padding buffer, and it intends that the buffer be newline and %NUL
> terminated.
>
> It looks very much like it wants to _avoid_ adding %NUL termination when
> doing copies, which is why it's using strncpy with a length argument of
> the source string length: it's _forcing_ the copy to not be terminated.
> This is just memcpy.
>
> strtomem() is designed for buffer sizes that can be known at compile
> time, so it's not useful here (as was found), since a string is being
> built up and uses a moving pointer.
>
> I think the correct fix is to use memcpy() instead of strscpy(). No
> %NUL-truncation is desired, the sizes are already determined and bounds
> checked. (And the latter is what likely silenced the compiler warning.)
>
> -Kees
Yes, your guess is right, we want to copy the string without termination.
Thanks for your introduction, we understand why strtomem() is not 
userful here.

Regards
Jijie Shao
Leon Romanovsky Aug. 13, 2023, 9:05 a.m. UTC | #8
On Thu, Aug 10, 2023 at 11:23:46AM -0700, Kees Cook wrote:
> On Thu, Aug 10, 2023 at 10:22:47AM -0700, Jakub Kicinski wrote:
> > 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@huawei.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..
> 
> tldr: use memcpy() instead of strscpy().
> 
> 
> Okay, I went to go read up on the history here. For my own notes, here's
> the original code, prior to 1cf3d5567f27 ("net: hns3: fix strncpy()
> not using dest-buf length as length issue"):
> 
> static void hns3_dbg_fill_content(char *content, u16 len,
> 				  const struct hns3_dbg_item *items,
> 				  const char **result, u16 size)
> {
> 	char *pos = content;
> 	u16 i;
> 
> 	memset(content, ' ', len);
> 	for (i = 0; i < size; i++) {
> 		if (result)
> 			strncpy(pos, result[i], strlen(result[i]));
> 		else
> 			strncpy(pos, items[i].name, strlen(items[i].name));
> 
> 		pos += strlen(items[i].name) + items[i].interval;
> 	}
> 
> 	*pos++ = '\n';
> 	*pos++ = '\0';
> }
> 
> The warning to be fixed was:
> 
> hclge_debugfs.c:90:25: warning: 'strncpy' output truncated before terminating nul copying as many bytes from a string as its length [-Wstringop-truncation]
> 
> There are a few extra checks added in 1cf3d5567f27, but I'm more curious
> about this original code's intent. It seems very confusing to me.
> 
> Firstly, why is "pos" updated based on "strlen(items[i].name)" even when
> "result[i]" is used? Secondly, why is "interval" used? (These concerns
> are mostly addressed in 1cf3d5567f27.)
> 
> I guess I'd just like to take a step back and ask, "What is this
> function trying to do?" It seems to be building a series of strings in a
> " "-padding buffer, and it intends that the buffer be newline and %NUL
> terminated.
> 
> It looks very much like it wants to _avoid_ adding %NUL termination when
> doing copies, which is why it's using strncpy with a length argument of
> the source string length: it's _forcing_ the copy to not be terminated.
> This is just memcpy.
> 
> strtomem() is designed for buffer sizes that can be known at compile
> time, so it's not useful here (as was found), since a string is being
> built up and uses a moving pointer.
> 
> I think the correct fix is to use memcpy() instead of strscpy(). No
> %NUL-truncation is desired, the sizes are already determined and bounds
> checked. (And the latter is what likely silenced the compiler warning.)

Thanks for an explanation.

> 
> -Kees
> 
> -- 
> Kees Cook
diff mbox series

Patch

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
index 52546f625c8b..f276b5ecb431 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
@@ -464,9 +464,9 @@  static void hns3_dbg_fill_content(char *content, u16 len,
 		if (result) {
 			if (item_len < strlen(result[i]))
 				break;
-			strscpy(pos, result[i], strlen(result[i]));
+			memcpy(pos, result[i], strlen(result[i]));
 		} else {
-			strscpy(pos, items[i].name, strlen(items[i].name));
+			memcpy(pos, items[i].name, strlen(items[i].name));
 		}
 		pos += item_len;
 		len -= item_len;
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c
index 409db2e70965..0fb2eaee3e8a 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c
@@ -111,9 +111,9 @@  static void hclge_dbg_fill_content(char *content, u16 len,
 		if (result) {
 			if (item_len < strlen(result[i]))
 				break;
-			strscpy(pos, result[i], strlen(result[i]));
+			memcpy(pos, result[i], strlen(result[i]));
 		} else {
-			strscpy(pos, items[i].name, strlen(items[i].name));
+			memcpy(pos, items[i].name, strlen(items[i].name));
 		}
 		pos += item_len;
 		len -= item_len;