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 11:35:55PM +0800, Hao Lan wrote:
> 
> 
> On 2023/5/16 22:11, Simon Horman wrote:
> > 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.
> > 
> Hi,
> Thanks for your review.
> Here is a warning report about using strcpy. This patch is used to fix the warning.
> Link: https://lore.kernel.org/lkml/202207170606.7WtHs9yS-lkp@intel.com/T/

Thanks.

Just to clarify, I was suggesting strcpy() rather than strncpy().
But I didn't try my suggestion.

In any case, all these ideas, including memcpy() have the underlying
issue that is being flagged below: that the length of the copy is based on
the source not the destination.

As I said earlier, I agree that your changes make this safe,
regardless of what function is used to actually copy the data.
And my suggestion is cosmetic rather than functional.

> All warnings (new ones prefixed by >>):
> 
>    In function 'hclge_dbg_fill_content',
>        inlined from 'hclge_dbg_dump_vlan_filter_config.constprop' at drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c:2080:2:
>    drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c:92:25: warning: 'strncpy' specified bound depends on the length of the source argument [-Wstringop-truncation]
>       92 |                         strncpy(pos, items[i].name, strlen(items[i].name));
>          |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    In function 'hclge_dbg_fill_content',
>        inlined from 'hclge_dbg_dump_vlan_filter_config.constprop' at drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c:2102:3:
> >> drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c:90:25: warning: 'strncpy' output truncated before terminating nul copying as many bytes from a string as its length [-Wstringop-truncation]
>       90 |                         strncpy(pos, result[i], strlen(result[i]));
>          |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    In function 'hclge_dbg_fill_content',
>        inlined from 'hclge_dbg_dump_mac_list' at drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c:1872:2:
>    drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c:92:25: warning: 'strncpy' specified bound depends on the length of the source argument [-Wstringop-truncation]
>       92 |                         strncpy(pos, items[i].name, strlen(items[i].name));
>          |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    In function 'hclge_dbg_fill_content',
>        inlined from 'hclge_dbg_dump_mac_list' at drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c:1887:4:
>    drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c:90:25: warning: 'strncpy' specified bound depends on the length of the source argument [-Wstringop-truncation]
>       90 |                         strncpy(pos, result[i], strlen(result[i]));
>          |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    In function 'hclge_dbg_fill_content',
>        inlined from 'hclge_dbg_dump_tm_pg' at drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c:735:2:
>    drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c:92:25: warning: 'strncpy' specified bound depends on the length of the source argument [-Wstringop-truncation]
>       92 |                         strncpy(pos, items[i].name, strlen(items[i].name));
>          |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    In function 'hclge_dbg_fill_content',
>        inlined from 'hclge_dbg_dump_tm_pg' at drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c:775:3:
>    drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c:90:25: warning: 'strncpy' specified bound depends on the length of the source argument [-Wstringop-truncation]
>       90 |                         strncpy(pos, result[i], strlen(result[i]));
>          |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    In function 'hclge_dbg_fill_content',
>        inlined from 'hclge_dbg_dump_tm_qset' at drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c:1027:2:
>    drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c:92:25: warning: 'strncpy' specified bound depends on the length of the source argument [-Wstringop-truncation]
>       92 |                         strncpy(pos, items[i].name, strlen(items[i].name));
>          |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    In function 'hclge_dbg_fill_content',
>        inlined from 'hclge_dbg_dump_tm_qset' at drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c:1059:3:
>    drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c:90:25: warning: 'strncpy' specified bound depends on the length of the source argument [-Wstringop-truncation]
>       90 |                         strncpy(pos, result[i], strlen(result[i]));
>          |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    In function 'hclge_dbg_fill_content',
>        inlined from 'hclge_dbg_dump_vlan_offload_config' at drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c:2123:2:
>    drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c:92:25: warning: 'strncpy' specified bound depends on the length of the source argument [-Wstringop-truncation]
>       92 |                         strncpy(pos, items[i].name, strlen(items[i].name));
>          |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    In function 'hclge_dbg_fill_content',
>        inlined from 'hclge_dbg_dump_vlan_offload_config' at drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c:2154:3:
> >> drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c:90:25: warning: 'strncpy' output truncated before terminating nul copying as many bytes from a string as its length [-Wstringop-truncation]
>       90 |                         strncpy(pos, result[i], strlen(result[i]));
>          |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> > ...
> > .
> >