| 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
| ||
|
Date: Tue, 16 May 2023 21:09:45 +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 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?
Thank you
>> + }
>> + pos += item_len;
>> + len -= item_len;
>> }
>> -
>> *pos++ = '\n';
>> *pos++ = '\0';
>> }
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c
>> index a0b46e7d863e..1354fd0461f7 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c
>> @@ -88,16 +88,35 @@ static void hclge_dbg_fill_content(char *content, u16 len,
>> const struct hclge_dbg_item *items,
>> const char **result, u16 size)
>> {
>> +#define HCLGE_DBG_LINE_END_LEN 2
>> char *pos = content;
>> + u16 item_len;
>> u16 i;
>>
>> + if (!len) {
>> + return;
>> + } else if (len <= HCLGE_DBG_LINE_END_LEN) {
>> + *pos++ = '\0';
>> + return;
>> + }
>> +
>> memset(content, ' ', len);
>> + len -= HCLGE_DBG_LINE_END_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;
>> + 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));
>> + }
>> + pos += item_len;
>> + len -= item_len;
>> }
>> *pos++ = '\n';
>> *pos++ = '\0';
>> --
>> 2.30.0
>>
>>
> .
>
Powered by blists - more mailing lists