| Message ID | 20230612122358.10586-4-lanhao@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: hns3: There are some cleanup for the HNS3 ethernet driver | expand |
On Mon, 2023-06-12 at 20:23 +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. > > ChangeLog: > v1->v2: > Use strcpy substitutes for memcpy > suggested by Simon Horman. > > 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 d385ffc21876..0749515e270b 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 IMHO this macro should be defined outside the function (just before) for better readability. > 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; > + strcpy(pos, result[i]); > + } else { > + strcpy(pos, items[i].name); > + } > + pos += item_len; > + len -= item_len; I think it would be better using 'strscpy' instead of papering over more unsecure functions. Thanks, Paolo
On Mon, 2023-06-12 at 20:23 +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. > > ChangeLog: > v1->v2: > Use strcpy substitutes for memcpy > suggested by Simon Horman. In the next revision, please move the changelog after the '---' separator below, so that it will no included into the actual commit message. (Applies to all the patch in the series) Thanks! Paolo
On 2023/6/14 18:34, Paolo Abeni wrote: > On Mon, 2023-06-12 at 20:23 +0800, Hao Lan wrote: > > I think it would be better using 'strscpy' instead of papering over > more unsecure functions. > > Thanks, > > Paolo > > . > Hi Paolo Abeni, We accept your suggestion, thank you.
On 2023/6/14 18:34, Paolo Abeni wrote: > On Mon, 2023-06-12 at 20:23 +0800, Hao Lan wrote: >> From: Hao Chen <chenhao418@huawei.com> >> >> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c >> index d385ffc21876..0749515e270b 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 > > IMHO this macro should be defined outside the function (just before) > for better readability. > Hi Paolo Abeni, Thank you for your advice. We use this macro style elsewhere in our code. The style is consistent. Here we keep this macro style. Yours, Hao Lan
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c index d385ffc21876..0749515e270b 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; + strcpy(pos, result[i]); + } else { + strcpy(pos, 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..0a1cc7084602 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; + strcpy(pos, result[i]); + } else { + strcpy(pos, items[i].name); + } + pos += item_len; + len -= item_len; } *pos++ = '\n'; *pos++ = '\0';