[PATCH net-next 0/3] bnxt_en: address string truncation

[PATCH net-next 0/3] bnxt_en: address string truncation

[Simon Horman]

Hi,

This series addresses several string truncation issues that are flagged
by gcc-14. I do not have any reason to believe these are bugs, so I am
targeting this at net-next and have not provided Fixes tags.

---
Simon Horman (3):
      bnxt_en: check for fw_ver_str truncation
      bnxt_en: check for irq name truncation
      bnxt_en: avoid truncation of per rx run debugfs filename

 drivers/net/ethernet/broadcom/bnxt/bnxt.c         | 30 +++++++++++++++++------
 drivers/net/ethernet/broadcom/bnxt/bnxt_debugfs.c |  4 +--
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 23 +++++++++++------
 3 files changed, 40 insertions(+), 17 deletions(-)

base-commit: aba43bdfdccf15da1dfdc657bd9dada9010d77a4

[PATCH net-next 1/3] bnxt_en: check for fw_ver_str truncation

[Simon Horman]

Given the sizes of the buffers involved, it is theoretically
possible for fw_ver_str to be truncated. Detect this and
stop ethtool initialisation if this occurs.

Flagged by gcc-14:

  .../bnxt_ethtool.c: In function 'bnxt_ethtool_init':
  drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c:4144:32: warning: '%s' directive output may be truncated writing up to 31 bytes into a region of size 26 [-Wformat-truncation=]
   4144 |                          "/pkg %s", buf);
        |                                ^~   ~~~
  In function 'bnxt_get_pkgver',
      inlined from 'bnxt_ethtool_init' at .../bnxt_ethtool.c:5056:3:
  .../bnxt_ethtool.c:4143:17: note: 'snprintf' output between 6 and 37 bytes into a destination of size 31
   4143 |                 snprintf(bp->fw_ver_str + len, FW_VER_STR_LEN - len - 1,
        |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   4144 |                          "/pkg %s", buf);
        |                          ~~~~~~~~~~~~~~~

Compile tested only.

Signed-off-by: Simon Horman <horms@kernel.org>
---
It appears to me that size is underestimated by 1 byte -
it should be FW_VER_STR_LEN - offset rather than FW_VER_STR_LEN - offset - 1,
because the size argument to snprintf should include the space for the
trailing '\0'. But I have not changed that as it is separate from
the issue this patch addresses.
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index bf157f6cc042..5ccc3cc4ba7d 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -4132,17 +4132,23 @@ int bnxt_get_pkginfo(struct net_device *dev, char *ver, int size)
 	return rc;
 }
 
-static void bnxt_get_pkgver(struct net_device *dev)
+static int bnxt_get_pkgver(struct net_device *dev)
 {
 	struct bnxt *bp = netdev_priv(dev);
 	char buf[FW_VER_STR_LEN];
-	int len;
 
 	if (!bnxt_get_pkginfo(dev, buf, sizeof(buf))) {
-		len = strlen(bp->fw_ver_str);
-		snprintf(bp->fw_ver_str + len, FW_VER_STR_LEN - len - 1,
-			 "/pkg %s", buf);
+		int offset, size, rc;
+
+		offset = strlen(bp->fw_ver_str);
+		size = FW_VER_STR_LEN - offset - 1;
+
+		rc = snprintf(bp->fw_ver_str + offset, size, "/pkg %s", buf);
+		if (rc >= size)
+			return -E2BIG;
 	}
+
+	return 0;
 }
 
 static int bnxt_get_eeprom(struct net_device *dev,
@@ -5052,8 +5058,11 @@ void bnxt_ethtool_init(struct bnxt *bp)
 	struct net_device *dev = bp->dev;
 	int i, rc;
 
-	if (!(bp->fw_cap & BNXT_FW_CAP_PKG_VER))
-		bnxt_get_pkgver(dev);
+	if (!(bp->fw_cap & BNXT_FW_CAP_PKG_VER)) {
+		rc = bnxt_get_pkgver(dev);
+		if (rc)
+			return;
+	}
 
 	bp->num_tests = 0;
 	if (bp->hwrm_spec_code < 0x10704 || !BNXT_PF(bp))

-- 
2.43.0

[PATCH net-next 2/3] bnxt_en: check for irq name truncation

[Simon Horman]

Given the sizes of the buffers involved, it is theoretically
possible for irq names to be truncated. Detect this and
propagate an error if this occurs.

Flagged by gcc-14:

  .../bnxt.c: In function 'bnxt_setup_int_mode':
  .../bnxt.c:10584:48: warning: '%s' directive output may be truncated writing 4 bytes into a region of size between 2 and 17 [-Wformat-truncation=]
  10584 |         snprintf(bp->irq_tbl[0].name, len, "%s-%s-%d", bp->dev->name, "TxRx",
        |                                                ^~                     ~~~~~~
  In function 'bnxt_setup_inta',
      inlined from 'bnxt_setup_int_mode' at .../bnxt.c:10604:3:
  .../bnxt.c:10584:9: note: 'snprintf' output between 8 and 23 bytes into a destination of size 18
  10584 |         snprintf(bp->irq_tbl[0].name, len, "%s-%s-%d", bp->dev->name, "TxRx",
        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  10585 |                  0);
        |                  ~~
  .../bnxt.c: In function 'bnxt_setup_int_mode':
  .../bnxt.c:10569:62: warning: '%s' directive output may be truncated writing between 2 and 4 bytes into a region of size between 2 and 17 [-Wformat-truncation=]
  10569 |                 snprintf(bp->irq_tbl[map_idx].name, len, "%s-%s-%d", dev->name,
        |                                                              ^~
  In function 'bnxt_setup_msix',
      inlined from 'bnxt_setup_int_mode' at .../bnxt.c:10602:3:
  .../bnxt.c:10569:58: note: directive argument in the range [-2147483643, 2147483646]
  10569 |                 snprintf(bp->irq_tbl[map_idx].name, len, "%s-%s-%d", dev->name,
        |                                                          ^~~~~~~~~~
  .../bnxt.c:10569:17: note: 'snprintf' output between 6 and 33 bytes into a destination of size 18
  10569 |                 snprintf(bp->irq_tbl[map_idx].name, len, "%s-%s-%d", dev->name,
        |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  10570 |                          attr, i);
        |                          ~~~~~~~~

Compile tested only.

Signed-off-by: Simon Horman <horms@kernel.org>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 220d05e2f6fa..15e68c8e599d 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -10538,7 +10538,7 @@ static int bnxt_trim_rings(struct bnxt *bp, int *rx, int *tx, int max,
 	return __bnxt_trim_rings(bp, rx, tx, max, sh);
 }
 
-static void bnxt_setup_msix(struct bnxt *bp)
+static int bnxt_setup_msix(struct bnxt *bp)
 {
 	const int len = sizeof(bp->irq_tbl[0].name);
 	struct net_device *dev = bp->dev;
@@ -10558,6 +10558,7 @@ static void bnxt_setup_msix(struct bnxt *bp)
 	for (i = 0; i < bp->cp_nr_rings; i++) {
 		int map_idx = bnxt_cp_num_to_irq_num(bp, i);
 		char *attr;
+		int rc;
 
 		if (bp->flags & BNXT_FLAG_SHARED_RINGS)
 			attr = "TxRx";
@@ -10566,24 +10567,35 @@ static void bnxt_setup_msix(struct bnxt *bp)
 		else
 			attr = "tx";
 
-		snprintf(bp->irq_tbl[map_idx].name, len, "%s-%s-%d", dev->name,
-			 attr, i);
+		rc = snprintf(bp->irq_tbl[map_idx].name, len, "%s-%s-%d",
+			      dev->name, attr, i);
+		if (rc >= len)
+			return -E2BIG;
 		bp->irq_tbl[map_idx].handler = bnxt_msix;
 	}
+
+	return 0;
 }
 
-static void bnxt_setup_inta(struct bnxt *bp)
+static int bnxt_setup_inta(struct bnxt *bp)
 {
 	const int len = sizeof(bp->irq_tbl[0].name);
+	int rc;
+
 
 	if (bp->num_tc) {
 		netdev_reset_tc(bp->dev);
 		bp->num_tc = 0;
 	}
 
-	snprintf(bp->irq_tbl[0].name, len, "%s-%s-%d", bp->dev->name, "TxRx",
-		 0);
+	rc = snprintf(bp->irq_tbl[0].name, len, "%s-%s-%d", bp->dev->name,
+		      "TxRx", 0);
+	if (rc >= len)
+		return -E2BIG;
+
 	bp->irq_tbl[0].handler = bnxt_inta;
+
+	return 0;
 }
 
 static int bnxt_init_int_mode(struct bnxt *bp);
@@ -10599,9 +10611,11 @@ static int bnxt_setup_int_mode(struct bnxt *bp)
 	}
 
 	if (bp->flags & BNXT_FLAG_USING_MSIX)
-		bnxt_setup_msix(bp);
+		rc = bnxt_setup_msix(bp);
 	else
-		bnxt_setup_inta(bp);
+		rc = bnxt_setup_inta(bp);
+	if (rc)
+		return rc;
 
 	rc = bnxt_set_real_num_queues(bp);
 	return rc;

-- 
2.43.0

[PATCH net-next 3/3] bnxt_en: avoid truncation of per rx run debugfs filename

[Simon Horman]

Although it seems unlikely in practice - there would need to be
rx ring indexes greater than 10^10 - it is theoretically possible
for the filename of per rx ring debugfs files to be truncated.

This is because although a 16 byte buffer is provided, the length
of the filename is restricted to 10 bytes. Remove this restriction
and allow the entire buffer to be used.

Also reduce the buffer to 12 bytes, which is sufficient.

Given that the range of rx ring indexes likely much smaller than the
maximum range of a 32-bit signed integer, a smaller buffer could be
used, with some further changes.  But this change seems simple, robust,
and has minimal stack overhead.

Flagged by gcc-14:

  .../bnxt_debugfs.c: In function 'bnxt_debug_dev_init':
  drivers/net/ethernet/broadcom/bnxt/bnxt_debugfs.c:69:30: warning: '%d' directive output may be truncated writing between 1 and 11 bytes into a region of size 10 [-Wformat-truncation=]
     69 |         snprintf(qname, 10, "%d", ring_idx);
        |                              ^~
  In function 'debugfs_dim_ring_init',
      inlined from 'bnxt_debug_dev_init' at .../bnxt_debugfs.c:87:4:
  .../bnxt_debugfs.c:69:29: note: directive argument in the range [-2147483643, 2147483646]
     69 |         snprintf(qname, 10, "%d", ring_idx);
        |                             ^~~~
  .../bnxt_debugfs.c:69:9: note: 'snprintf' output between 2 and 12 bytes into a destination of size 10
     69 |         snprintf(qname, 10, "%d", ring_idx);
        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Compile tested only

Signed-off-by: Simon Horman <horms@kernel.org>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_debugfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_debugfs.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_debugfs.c
index 156c2404854f..127b7015f676 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_debugfs.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_debugfs.c
@@ -64,9 +64,9 @@ static const struct file_operations debugfs_dim_fops = {
 static void debugfs_dim_ring_init(struct dim *dim, int ring_idx,
 				  struct dentry *dd)
 {
-	static char qname[16];
+	static char qname[12];
 
-	snprintf(qname, 10, "%d", ring_idx);
+	snprintf(qname, sizeof(qname), "%d", ring_idx);
 	debugfs_create_file(qname, 0600, dd, dim, &debugfs_dim_fops);
 }
 

-- 
2.43.0

Re: [PATCH net-next 1/3] bnxt_en: check for fw_ver_str truncation

[Przemek Kitszel]

On 7/5/24 13:26, Simon Horman wrote:
> Given the sizes of the buffers involved, it is theoretically
> possible for fw_ver_str to be truncated. Detect this and
> stop ethtool initialisation if this occurs.
> 
> Flagged by gcc-14:
> 
>    .../bnxt_ethtool.c: In function 'bnxt_ethtool_init':
>    drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c:4144:32: warning: '%s' directive output may be truncated writing up to 31 bytes into a region of size 26 [-Wformat-truncation=]
>     4144 |                          "/pkg %s", buf);
>          |                                ^~   ~~~

gcc is right, and you are right that we don't want such warnings
but I believe that the current flow is fine (copy as much as possible,
then proceed)

>    In function 'bnxt_get_pkgver',
>        inlined from 'bnxt_ethtool_init' at .../bnxt_ethtool.c:5056:3:
>    .../bnxt_ethtool.c:4143:17: note: 'snprintf' output between 6 and 37 bytes into a destination of size 31
>     4143 |                 snprintf(bp->fw_ver_str + len, FW_VER_STR_LEN - len - 1,
>          |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     4144 |                          "/pkg %s", buf);
>          |                          ~~~~~~~~~~~~~~~
> 
> Compile tested only.
> 
> Signed-off-by: Simon Horman <horms@kernel.org>
> ---
> It appears to me that size is underestimated by 1 byte -
> it should be FW_VER_STR_LEN - offset rather than FW_VER_STR_LEN - offset - 1,
> because the size argument to snprintf should include the space for the
> trailing '\0'. But I have not changed that as it is separate from
> the issue this patch addresses.

you are addressing "bad size" for copying strings around, I will just
fix that part too

> ---
>   drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 23 ++++++++++++++++-------
>   1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index bf157f6cc042..5ccc3cc4ba7d 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -4132,17 +4132,23 @@ int bnxt_get_pkginfo(struct net_device *dev, char *ver, int size)
>   	return rc;
>   }
>   
> -static void bnxt_get_pkgver(struct net_device *dev)
> +static int bnxt_get_pkgver(struct net_device *dev)
>   {
>   	struct bnxt *bp = netdev_priv(dev);
>   	char buf[FW_VER_STR_LEN];
> -	int len;
>   
>   	if (!bnxt_get_pkginfo(dev, buf, sizeof(buf))) {
> -		len = strlen(bp->fw_ver_str);
> -		snprintf(bp->fw_ver_str + len, FW_VER_STR_LEN - len - 1,
> -			 "/pkg %s", buf);
> +		int offset, size, rc;
> +
> +		offset = strlen(bp->fw_ver_str);
> +		size = FW_VER_STR_LEN - offset - 1;
> +
> +		rc = snprintf(bp->fw_ver_str + offset, size, "/pkg %s", buf);
> +		if (rc >= size)
> +			return -E2BIG;

On error I would just replace last few bytes with "(...)" or "...", or
even "~". Other option is to enlarge bp->fw_ver_str, but I have not
looked there.

>   	}
> +
> +	return 0;
>   }
>   
>   static int bnxt_get_eeprom(struct net_device *dev,
> @@ -5052,8 +5058,11 @@ void bnxt_ethtool_init(struct bnxt *bp)
>   	struct net_device *dev = bp->dev;
>   	int i, rc;
>   
> -	if (!(bp->fw_cap & BNXT_FW_CAP_PKG_VER))
> -		bnxt_get_pkgver(dev);
> +	if (!(bp->fw_cap & BNXT_FW_CAP_PKG_VER)) {
> +		rc = bnxt_get_pkgver(dev);
> +		if (rc)
> +			return;

and here you are changing the flow, I would like to still init the
rest of the bnxt' ethtool stuff despite one informative string
being turncated

> +	}
>   
>   	bp->num_tests = 0;
>   	if (bp->hwrm_spec_code < 0x10704 || !BNXT_PF(bp))
> 

Re: [PATCH net-next 1/3] bnxt_en: check for fw_ver_str truncation

[Simon Horman]

On Fri, Jul 05, 2024 at 02:37:58PM +0200, Przemek Kitszel wrote:
> On 7/5/24 13:26, Simon Horman wrote:
> > Given the sizes of the buffers involved, it is theoretically
> > possible for fw_ver_str to be truncated. Detect this and
> > stop ethtool initialisation if this occurs.
> > 
> > Flagged by gcc-14:
> > 
> >    .../bnxt_ethtool.c: In function 'bnxt_ethtool_init':
> >    drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c:4144:32: warning: '%s' directive output may be truncated writing up to 31 bytes into a region of size 26 [-Wformat-truncation=]
> >     4144 |                          "/pkg %s", buf);
> >          |                                ^~   ~~~
> 
> gcc is right, and you are right that we don't want such warnings
> but I believe that the current flow is fine (copy as much as possible,
> then proceed)
> 
> >    In function 'bnxt_get_pkgver',
> >        inlined from 'bnxt_ethtool_init' at .../bnxt_ethtool.c:5056:3:
> >    .../bnxt_ethtool.c:4143:17: note: 'snprintf' output between 6 and 37 bytes into a destination of size 31
> >     4143 |                 snprintf(bp->fw_ver_str + len, FW_VER_STR_LEN - len - 1,
> >          |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >     4144 |                          "/pkg %s", buf);
> >          |                          ~~~~~~~~~~~~~~~
> > 
> > Compile tested only.
> > 
> > Signed-off-by: Simon Horman <horms@kernel.org>
> > ---
> > It appears to me that size is underestimated by 1 byte -
> > it should be FW_VER_STR_LEN - offset rather than FW_VER_STR_LEN - offset - 1,
> > because the size argument to snprintf should include the space for the
> > trailing '\0'. But I have not changed that as it is separate from
> > the issue this patch addresses.
> 
> you are addressing "bad size" for copying strings around, I will just
> fix that part too

Right, I was thinking of handling that separately.

> > ---
> >   drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 23 ++++++++++++++++-------
> >   1 file changed, 16 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > index bf157f6cc042..5ccc3cc4ba7d 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > @@ -4132,17 +4132,23 @@ int bnxt_get_pkginfo(struct net_device *dev, char *ver, int size)
> >   	return rc;
> >   }
> > -static void bnxt_get_pkgver(struct net_device *dev)
> > +static int bnxt_get_pkgver(struct net_device *dev)
> >   {
> >   	struct bnxt *bp = netdev_priv(dev);
> >   	char buf[FW_VER_STR_LEN];
> > -	int len;
> >   	if (!bnxt_get_pkginfo(dev, buf, sizeof(buf))) {
> > -		len = strlen(bp->fw_ver_str);
> > -		snprintf(bp->fw_ver_str + len, FW_VER_STR_LEN - len - 1,
> > -			 "/pkg %s", buf);
> > +		int offset, size, rc;
> > +
> > +		offset = strlen(bp->fw_ver_str);
> > +		size = FW_VER_STR_LEN - offset - 1;
> > +
> > +		rc = snprintf(bp->fw_ver_str + offset, size, "/pkg %s", buf);
> > +		if (rc >= size)
> > +			return -E2BIG;
> 
> On error I would just replace last few bytes with "(...)" or "...", or
> even "~". Other option is to enlarge bp->fw_ver_str, but I have not
> looked there.
> 
> >   	}
> > +
> > +	return 0;
> >   }
> >   static int bnxt_get_eeprom(struct net_device *dev,
> > @@ -5052,8 +5058,11 @@ void bnxt_ethtool_init(struct bnxt *bp)
> >   	struct net_device *dev = bp->dev;
> >   	int i, rc;
> > -	if (!(bp->fw_cap & BNXT_FW_CAP_PKG_VER))
> > -		bnxt_get_pkgver(dev);
> > +	if (!(bp->fw_cap & BNXT_FW_CAP_PKG_VER)) {
> > +		rc = bnxt_get_pkgver(dev);
> > +		if (rc)
> > +			return;
> 
> and here you are changing the flow, I would like to still init the
> rest of the bnxt' ethtool stuff despite one informative string
> being turncated

Thanks, I'm fine with your suggestion.
But I'll wait to see if there is feedback from others, especially Broadcom.

> > +	}
> >   	bp->num_tests = 0;
> >   	if (bp->hwrm_spec_code < 0x10704 || !BNXT_PF(bp))
> > 
> 

Re: [PATCH net-next 1/3] bnxt_en: check for fw_ver_str truncation

[Pavan Chebbi]

[-- Attachment #1: Type: text/plain, Size: 4694 bytes --]

On Fri, Jul 5, 2024 at 9:36 PM Simon Horman <horms@kernel.org> wrote:
>
> On Fri, Jul 05, 2024 at 02:37:58PM +0200, Przemek Kitszel wrote:
> > On 7/5/24 13:26, Simon Horman wrote:
> > > Given the sizes of the buffers involved, it is theoretically
> > > possible for fw_ver_str to be truncated. Detect this and
> > > stop ethtool initialisation if this occurs.
> > >
> > > Flagged by gcc-14:
> > >
> > >    .../bnxt_ethtool.c: In function 'bnxt_ethtool_init':
> > >    drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c:4144:32: warning: '%s' directive output may be truncated writing up to 31 bytes into a region of size 26 [-Wformat-truncation=]
> > >     4144 |                          "/pkg %s", buf);
> > >          |                                ^~   ~~~
> >
> > gcc is right, and you are right that we don't want such warnings
> > but I believe that the current flow is fine (copy as much as possible,
> > then proceed)
> >
> > >    In function 'bnxt_get_pkgver',
> > >        inlined from 'bnxt_ethtool_init' at .../bnxt_ethtool.c:5056:3:
> > >    .../bnxt_ethtool.c:4143:17: note: 'snprintf' output between 6 and 37 bytes into a destination of size 31
> > >     4143 |                 snprintf(bp->fw_ver_str + len, FW_VER_STR_LEN - len - 1,
> > >          |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >     4144 |                          "/pkg %s", buf);
> > >          |                          ~~~~~~~~~~~~~~~
> > >
> > > Compile tested only.
> > >
> > > Signed-off-by: Simon Horman <horms@kernel.org>
> > > ---
> > > It appears to me that size is underestimated by 1 byte -
> > > it should be FW_VER_STR_LEN - offset rather than FW_VER_STR_LEN - offset - 1,
> > > because the size argument to snprintf should include the space for the
> > > trailing '\0'. But I have not changed that as it is separate from
> > > the issue this patch addresses.
> >
> > you are addressing "bad size" for copying strings around, I will just
> > fix that part too
>
> Right, I was thinking of handling that separately.
>
> > > ---
> > >   drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 23 ++++++++++++++++-------
> > >   1 file changed, 16 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > > index bf157f6cc042..5ccc3cc4ba7d 100644
> > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > > @@ -4132,17 +4132,23 @@ int bnxt_get_pkginfo(struct net_device *dev, char *ver, int size)
> > >     return rc;
> > >   }
> > > -static void bnxt_get_pkgver(struct net_device *dev)
> > > +static int bnxt_get_pkgver(struct net_device *dev)
> > >   {
> > >     struct bnxt *bp = netdev_priv(dev);
> > >     char buf[FW_VER_STR_LEN];
> > > -   int len;
> > >     if (!bnxt_get_pkginfo(dev, buf, sizeof(buf))) {
> > > -           len = strlen(bp->fw_ver_str);
> > > -           snprintf(bp->fw_ver_str + len, FW_VER_STR_LEN - len - 1,
> > > -                    "/pkg %s", buf);
> > > +           int offset, size, rc;
> > > +
> > > +           offset = strlen(bp->fw_ver_str);
> > > +           size = FW_VER_STR_LEN - offset - 1;
> > > +
> > > +           rc = snprintf(bp->fw_ver_str + offset, size, "/pkg %s", buf);
> > > +           if (rc >= size)
> > > +                   return -E2BIG;
> >
> > On error I would just replace last few bytes with "(...)" or "...", or
> > even "~". Other option is to enlarge bp->fw_ver_str, but I have not
> > looked there.
> >
> > >     }
> > > +
> > > +   return 0;
> > >   }
> > >   static int bnxt_get_eeprom(struct net_device *dev,
> > > @@ -5052,8 +5058,11 @@ void bnxt_ethtool_init(struct bnxt *bp)
> > >     struct net_device *dev = bp->dev;
> > >     int i, rc;
> > > -   if (!(bp->fw_cap & BNXT_FW_CAP_PKG_VER))
> > > -           bnxt_get_pkgver(dev);
> > > +   if (!(bp->fw_cap & BNXT_FW_CAP_PKG_VER)) {
> > > +           rc = bnxt_get_pkgver(dev);
> > > +           if (rc)
> > > +                   return;
> >
> > and here you are changing the flow, I would like to still init the
> > rest of the bnxt' ethtool stuff despite one informative string
> > being turncated
>
> Thanks, I'm fine with your suggestion.
> But I'll wait to see if there is feedback from others, especially Broadcom.

Hi Simon, thanks for the patch. I'd agree with Przemek. Would
definitely like to have the init complete just as before.

>
> > > +   }
> > >     bp->num_tests = 0;
> > >     if (bp->hwrm_spec_code < 0x10704 || !BNXT_PF(bp))
> > >
> >
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net-next 1/3] bnxt_en: check for fw_ver_str truncation

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 1848 bytes --]

On Fri, Jul 5, 2024 at 10:03 AM Pavan Chebbi <pavan.chebbi@broadcom.com> wrote:
>
> On Fri, Jul 5, 2024 at 9:36 PM Simon Horman <horms@kernel.org> wrote:
> >
> > On Fri, Jul 05, 2024 at 02:37:58PM +0200, Przemek Kitszel wrote:
> > > On 7/5/24 13:26, Simon Horman wrote:
> > > > It appears to me that size is underestimated by 1 byte -
> > > > it should be FW_VER_STR_LEN - offset rather than FW_VER_STR_LEN - offset - 1,
> > > > because the size argument to snprintf should include the space for the
> > > > trailing '\0'. But I have not changed that as it is separate from
> > > > the issue this patch addresses.
> > >
> > > you are addressing "bad size" for copying strings around, I will just
> > > fix that part too
> >
> > Right, I was thinking of handling that separately.

Yes, please fix the size as well.

> > > >   static int bnxt_get_eeprom(struct net_device *dev,
> > > > @@ -5052,8 +5058,11 @@ void bnxt_ethtool_init(struct bnxt *bp)
> > > >     struct net_device *dev = bp->dev;
> > > >     int i, rc;
> > > > -   if (!(bp->fw_cap & BNXT_FW_CAP_PKG_VER))
> > > > -           bnxt_get_pkgver(dev);
> > > > +   if (!(bp->fw_cap & BNXT_FW_CAP_PKG_VER)) {
> > > > +           rc = bnxt_get_pkgver(dev);
> > > > +           if (rc)
> > > > +                   return;
> > >
> > > and here you are changing the flow, I would like to still init the
> > > rest of the bnxt' ethtool stuff despite one informative string
> > > being turncated
> >
> > Thanks, I'm fine with your suggestion.
> > But I'll wait to see if there is feedback from others, especially Broadcom.
>
> Hi Simon, thanks for the patch. I'd agree with Przemek. Would
> definitely like to have the init complete just as before.
>

I agree as well.  We should continue with the rest of
bnxt_ethtool_init().  Thanks for the patch.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net-next 2/3] bnxt_en: check for irq name truncation

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 1679 bytes --]

On Fri, Jul 5, 2024 at 4:27 AM Simon Horman <horms@kernel.org> wrote:
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 220d05e2f6fa..15e68c8e599d 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -10538,7 +10538,7 @@ static int bnxt_trim_rings(struct bnxt *bp, int *rx, int *tx, int max,
>         return __bnxt_trim_rings(bp, rx, tx, max, sh);
>  }
>
> -static void bnxt_setup_msix(struct bnxt *bp)
> +static int bnxt_setup_msix(struct bnxt *bp)
>  {
>         const int len = sizeof(bp->irq_tbl[0].name);
>         struct net_device *dev = bp->dev;
> @@ -10558,6 +10558,7 @@ static void bnxt_setup_msix(struct bnxt *bp)
>         for (i = 0; i < bp->cp_nr_rings; i++) {
>                 int map_idx = bnxt_cp_num_to_irq_num(bp, i);
>                 char *attr;
> +               int rc;
>
>                 if (bp->flags & BNXT_FLAG_SHARED_RINGS)
>                         attr = "TxRx";
> @@ -10566,24 +10567,35 @@ static void bnxt_setup_msix(struct bnxt *bp)
>                 else
>                         attr = "tx";
>
> -               snprintf(bp->irq_tbl[map_idx].name, len, "%s-%s-%d", dev->name,
> -                        attr, i);
> +               rc = snprintf(bp->irq_tbl[map_idx].name, len, "%s-%s-%d",
> +                             dev->name, attr, i);
> +               if (rc >= len)
> +                       return -E2BIG;

I may be missing something obvious here.  snprintf() will truncate and
not overwrite the buffer, right?  Why is it necessary to abort if
there is truncation?  Thanks.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net-next 2/3] bnxt_en: check for irq name truncation

[Simon Horman]

On Fri, Jul 05, 2024 at 11:27:47AM -0700, Michael Chan wrote:
> On Fri, Jul 5, 2024 at 4:27 AM Simon Horman <horms@kernel.org> wrote:
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > index 220d05e2f6fa..15e68c8e599d 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > @@ -10538,7 +10538,7 @@ static int bnxt_trim_rings(struct bnxt *bp, int *rx, int *tx, int max,
> >         return __bnxt_trim_rings(bp, rx, tx, max, sh);
> >  }
> >
> > -static void bnxt_setup_msix(struct bnxt *bp)
> > +static int bnxt_setup_msix(struct bnxt *bp)
> >  {
> >         const int len = sizeof(bp->irq_tbl[0].name);
> >         struct net_device *dev = bp->dev;
> > @@ -10558,6 +10558,7 @@ static void bnxt_setup_msix(struct bnxt *bp)
> >         for (i = 0; i < bp->cp_nr_rings; i++) {
> >                 int map_idx = bnxt_cp_num_to_irq_num(bp, i);
> >                 char *attr;
> > +               int rc;
> >
> >                 if (bp->flags & BNXT_FLAG_SHARED_RINGS)
> >                         attr = "TxRx";
> > @@ -10566,24 +10567,35 @@ static void bnxt_setup_msix(struct bnxt *bp)
> >                 else
> >                         attr = "tx";
> >
> > -               snprintf(bp->irq_tbl[map_idx].name, len, "%s-%s-%d", dev->name,
> > -                        attr, i);
> > +               rc = snprintf(bp->irq_tbl[map_idx].name, len, "%s-%s-%d",
> > +                             dev->name, attr, i);
> > +               if (rc >= len)
> > +                       return -E2BIG;
> 
> I may be missing something obvious here.  snprintf() will truncate and
> not overwrite the buffer, right?  Why is it necessary to abort if
> there is truncation?  Thanks.

The (incorrect) assumption on my side was that truncated names
are undesirable and should be treated as an error case.
Sorry for not making that clearer.