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: Fri, 11 Aug 2023 10:28:08 +0800
From: Jijie Shao <shaojijie@...wei.com>
To: Kees Cook <keescook@...omium.org>
CC: <shaojijie@...wei.com>, 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>, Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net] net: hns3: fix strscpy causing content truncation
 issue


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