| 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: <ZGPUCiX06Y7/p6pZ@corigine.com>
Date: Tue, 16 May 2023 21:05:46 +0200
From: Simon Horman <simon.horman@...igine.com>
To: Hao Lan <lanhao@...wei.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 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@...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/
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]));
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> > ...
> > .
> >
Powered by blists - more mailing lists