netdev - [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 for Android: free password hash cracker in your pocket

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

Message-Id: <20240705-bnxt-str-v1-1-bafc769ed89e@kernel.org>
Date: Fri, 05 Jul 2024 12:26:47 +0100
From: Simon Horman <horms@...nel.org>
To: "David S. Miller" <davem@...emloft.net>, 
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
 Paolo Abeni <pabeni@...hat.com>
Cc: Michael Chan <michael.chan@...adcom.com>, netdev@...r.kernel.org
Subject: [PATCH net-next 1/3] bnxt_en: check for fw_ver_str truncation

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