bnx2x: Fix firmware version string character counts

[ Upstream commit 5642c82b9463c3263c086efb002516244bd4c668 ]

A potential string truncation was reported in bnx2x_fill_fw_str(),
when a long bp->fw_ver and a long phy_fw_ver might coexist, but seems
unlikely with real-world hardware.

Use scnprintf() to indicate the intent that truncations are tolerated.

While reading this code, I found a collection of various buffer size
counting issues. None looked like they might lead to a buffer overflow
with current code (the small buffers are 20 bytes and might only ever
consume 10 bytes twice with a trailing %NUL). However, early truncation
(due to a %NUL in the middle of the string) might be happening under
likely rare conditions. Regardless fix the formatters and related
functions:

- Switch from a separate strscpy() to just adding an additional "%s" to
  the format string that immediately follows it in bnx2x_fill_fw_str().
- Use sizeof() universally instead of using unbound defines.
- Fix bnx2x_7101_format_ver() and bnx2x_null_format_ver() to report the
  number of characters written, not including the trailing %NUL (as
  already done with the other firmware formatting functions).
- Require space for at least 1 byte in bnx2x_get_ext_phy_fw_version()
  for the trailing %NUL.
- Correct the needed buffer size in bnx2x_3_seq_format_ver().

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202401260858.jZN6vD1k-lkp@intel.com/
Cc: Ariel Elior <aelior@marvell.com>
Cc: Sudarsana Kalluru <skalluru@marvell.com>
Cc: Manish Chopra <manishc@marvell.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Link: https://lore.kernel.org/r/20240126041044.work.220-kees@kernel.org
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index e9c1e1bb5580601de46de29c7967d44ad39c260d..528441b28c4efe73eae5aa366c66b5ef8cc35ef2 100644 (file)
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -147,10 +147,11 @@ void bnx2x_fill_fw_str(struct bnx2x *bp, char *buf, size_t buf_len)
 
                phy_fw_ver[0] = '\0';
                bnx2x_get_ext_phy_fw_version(&bp->link_params,
-                                            phy_fw_ver, PHY_FW_VER_LEN);
-               strscpy(buf, bp->fw_ver, buf_len);
-               snprintf(buf + strlen(bp->fw_ver), 32 - strlen(bp->fw_ver),
-                        "bc %d.%d.%d%s%s",
+                                            phy_fw_ver, sizeof(phy_fw_ver));
+               /* This may become truncated. */
+               scnprintf(buf, buf_len,
+                        "%sbc %d.%d.%d%s%s",
+                        bp->fw_ver,
                         (bp->common.bc_ver & 0xff0000) >> 16,
                         (bp->common.bc_ver & 0xff00) >> 8,
                         (bp->common.bc_ver & 0xff),

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
index 81d232e6d05fe18b411dc3221c5abbfad17db462..0bc7690cdee169fc48844f2f465feb4ffb2b0ddc 100644 (file)
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
@@ -1132,7 +1132,7 @@ static void bnx2x_get_drvinfo(struct net_device *dev,
        }
 
        memset(version, 0, sizeof(version));
-       bnx2x_fill_fw_str(bp, version, ETHTOOL_FWVERS_LEN);
+       bnx2x_fill_fw_str(bp, version, sizeof(version));
        strlcat(info->fw_version, version, sizeof(info->fw_version));
 
        strscpy(info->bus_info, pci_name(bp->pdev), sizeof(info->bus_info));

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_link.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_link.c
index 02808513ffe45b7b09c4fee82e6e79f4849544c8..ea310057fe3aff71a091e1d5627b5f6e3f3b49b0 100644 (file)
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_link.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_link.c
@@ -6163,8 +6163,8 @@ static void bnx2x_link_int_ack(struct link_params *params,
 
 static int bnx2x_null_format_ver(u32 spirom_ver, u8 *str, u16 *len)
 {
-       str[0] = '\0';
-       (*len)--;
+       if (*len)
+               str[0] = '\0';
        return 0;
 }
 
@@ -6173,7 +6173,7 @@ static int bnx2x_format_ver(u32 num, u8 *str, u16 *len)
        u16 ret;
 
        if (*len < 10) {
-               /* Need more than 10chars for this format */
+               /* Need more than 10 chars for this format */
                bnx2x_null_format_ver(num, str, len);
                return -EINVAL;
        }
@@ -6188,8 +6188,8 @@ static int bnx2x_3_seq_format_ver(u32 num, u8 *str, u16 *len)
 {
        u16 ret;
 
-       if (*len < 10) {
-               /* Need more than 10chars for this format */
+       if (*len < 9) {
+               /* Need more than 9 chars for this format */
                bnx2x_null_format_ver(num, str, len);
                return -EINVAL;
        }
@@ -6208,7 +6208,7 @@ int bnx2x_get_ext_phy_fw_version(struct link_params *params, u8 *version,
        int status = 0;
        u8 *ver_p = version;
        u16 remain_len = len;
-       if (version == NULL || params == NULL)
+       if (version == NULL || params == NULL || len == 0)
                return -EINVAL;
        bp = params->bp;
 
@@ -11546,7 +11546,7 @@ static int bnx2x_7101_format_ver(u32 spirom_ver, u8 *str, u16 *len)
        str[2] = (spirom_ver & 0xFF0000) >> 16;
        str[3] = (spirom_ver & 0xFF000000) >> 24;
        str[4] = '\0';
-       *len -= 5;
+       *len -= 4;
        return 0;
 }