| Message ID | 20230515134643.48314-4-lanhao@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: hns3: There are some cleanup for the HNS3 ethernet driver | expand |
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. > + } > + 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 > >
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? 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 >> >> > . >
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. ...
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/ 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])); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ... > . >
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])); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > ... > > . > >
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)); + } + 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';