netdev - Re: [PATCH net-next 1/3] bnxt_en: check for fw_ver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240705160635.GA1480790@kernel.org>
Date: Fri, 5 Jul 2024 17:06:35 +0100
From: Simon Horman <horms@...nel.org>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>
Cc: Michael Chan <michael.chan@...adcom.com>, netdev@...r.kernel.org,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next 1/3] bnxt_en: check for fw_ver_str truncation

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@...nel.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))
> > 
>