| 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
| ||
|
Message-ID: <199c9494-e26c-f665-744f-6475ba7dae3a@huawei.com>
Date: Tue, 16 May 2023 23:35:55 +0800
From: Hao Lan <lanhao@...wei.com>
To: Simon Horman <simon.horman@...igine.com>
CC: <netdev@...r.kernel.org>, <yisen.zhuang@...wei.com>,
<salil.mehta@...wei.com>, <davem@...emloft.net>, <edumazet@...gle.com>,
<kuba@...nel.org>, <pabeni@...hat.com>, <richardcochran@...il.com>,
<wangpeiyang1@...wei.com>, <shenjian15@...wei.com>, <chenhao418@...wei.com>,
<wangjie125@...wei.com>, <yuanjilin@...rlc.com>, <cai.huoqing@...ux.dev>,
<xiujianfeng@...wei.com>
Subject: Re: [PATCH net-next 3/4] net: hns3: fix strncpy() not using dest-buf
length as length issue
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@...wei.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@...wei.com>
>>>> Reported-by: kernel test robot <lkp@...el.com>
>>>> Closes: https://lore.kernel.org/lkml/202207170606.7WtHs9yS-lkp@intel.com/T/
>>>> Signed-off-by: Hao Lan <lanhao@...wei.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/
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]));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ...
> .
>
Powered by blists - more mailing lists