Re: [PATCH net-next 3/4] net: hns3: fix strncpy() not using dest-buf length as length issue

[Simon Horman <simon.horman@corigine.com>]

On Tue, May 16, 2023 at 09:09:45PM +0800, Hao Lan wrote:
> 
> 
> On 2023/5/16 3:57, Simon Horman wrote:
> > On Mon, May 15, 2023 at 09:46:42PM +0800, Hao Lan wrote:
> >> From: Hao Chen <chenhao418@huawei.com>
> >>
> >> Now, strncpy() in hns3_dbg_fill_content() use src-length as copy-length,
> >> it may result in dest-buf overflow.
> >>
> >> This patch is to fix intel compile warning for csky-linux-gcc (GCC) 12.1.0
> >> compiler.
> >>
> >> The warning reports as below:
> >>
> >> hclge_debugfs.c:92:25: warning: 'strncpy' specified bound depends on
> >> the length of the source argument [-Wstringop-truncation]
> >>
> >> strncpy(pos, items[i].name, strlen(items[i].name));
> >>
> >> hclge_debugfs.c:90:25: warning: 'strncpy' output truncated before
> >> terminating nul copying as many bytes from a string as its length
> >> [-Wstringop-truncation]
> >>
> >> strncpy(pos, result[i], strlen(result[i]));
> >>
> >> strncpy() use src-length as copy-length, it may result in
> >> dest-buf overflow.
> >>
> >> So,this patch add some values check to avoid this issue.
> >>
> >> Signed-off-by: Hao Chen <chenhao418@huawei.com>
> >> Reported-by: kernel test robot <lkp@intel.com>
> >> Closes: https://lore.kernel.org/lkml/202207170606.7WtHs9yS-lkp@intel.com/T/
> >> Signed-off-by: Hao Lan <lanhao@huawei.com>
> >> ---
> >>  .../ethernet/hisilicon/hns3/hns3_debugfs.c    | 31 ++++++++++++++-----
> >>  .../hisilicon/hns3/hns3pf/hclge_debugfs.c     | 29 ++++++++++++++---
> >>  2 files changed, 48 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
> >> index 4c3e90a1c4d0..cf415cb37685 100644
> >> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
> >> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
> >> @@ -438,19 +438,36 @@ static void hns3_dbg_fill_content(char *content, u16 len,
> >>  				  const struct hns3_dbg_item *items,
> >>  				  const char **result, u16 size)
> >>  {
> >> +#define HNS3_DBG_LINE_END_LEN	2
> >>  	char *pos = content;
> >> +	u16 item_len;
> >>  	u16 i;
> >>  
> >> +	if (!len) {
> >> +		return;
> >> +	} else if (len <= HNS3_DBG_LINE_END_LEN) {
> >> +		*pos++ = '\0';
> >> +		return;
> >> +	}
> >> +
> >>  	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));
> >> +	len -= HNS3_DBG_LINE_END_LEN;
> >>  
> >> -		pos += strlen(items[i].name) + items[i].interval;
> >> +	for (i = 0; i < size; i++) {
> >> +		item_len = strlen(items[i].name) + items[i].interval;
> >> +		if (len < item_len)
> >> +			break;
> >> +
> >> +		if (result) {
> >> +			if (item_len < strlen(result[i]))
> >> +				break;
> >> +			memcpy(pos, result[i], strlen(result[i]));
> >> +		} else {
> >> +			memcpy(pos, items[i].name, strlen(items[i].name));
> > 
> > Hi,
> > 
> > The above memcpy() calls share the same property as the warning that
> > is being addressed: the length copied depends on the source not the
> > destination.
> > 
> > With the reworked code this seems safe. Which is good. But I wonder if,
> > given all the checking done, it makes sense to simply call strcpy() here.
> > Using strlen() as a length argument seems odd to me.
> > 
> Hi,
> Thanks for your review.
> 1. We think the memcpy is correct, our length copied depends on the source,
> or do I not understand you?
> void *memcpy(void *dest, const void *src, size_t count)
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/string.c#n619
> 
> 2. We don't know any other way to replace strlen. Do you have a better way for us?

My point is that strcpy() could be used here.
Because the source is a string, and the length of the copy
is the length of the source (string).

f.e., aren't the following functionally equivalent?

	memcpy(pos, result[i], strlen(result[i]));

	strcpy(pos, result[i])

In my view using strcpy here seems a bit simpler and therefore nicer.
But if you don't think so, that is fine.

...