Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

bnx2x: Fix firmware version string character counts

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>

authored by

Kees Cook and committed by
Jakub Kicinski
5642c82b 8d029330

+13 -12
+5 -4
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
··· 147 147 148 148 phy_fw_ver[0] = '\0'; 149 149 bnx2x_get_ext_phy_fw_version(&bp->link_params, 150 - phy_fw_ver, PHY_FW_VER_LEN); 151 - strscpy(buf, bp->fw_ver, buf_len); 152 - snprintf(buf + strlen(bp->fw_ver), 32 - strlen(bp->fw_ver), 153 - "bc %d.%d.%d%s%s", 150 + phy_fw_ver, sizeof(phy_fw_ver)); 151 + /* This may become truncated. */ 152 + scnprintf(buf, buf_len, 153 + "%sbc %d.%d.%d%s%s", 154 + bp->fw_ver, 154 155 (bp->common.bc_ver & 0xff0000) >> 16, 155 156 (bp->common.bc_ver & 0xff00) >> 8, 156 157 (bp->common.bc_ver & 0xff),
+1 -1
drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
··· 1132 1132 } 1133 1133 1134 1134 memset(version, 0, sizeof(version)); 1135 - bnx2x_fill_fw_str(bp, version, ETHTOOL_FWVERS_LEN); 1135 + bnx2x_fill_fw_str(bp, version, sizeof(version)); 1136 1136 strlcat(info->fw_version, version, sizeof(info->fw_version)); 1137 1137 1138 1138 strscpy(info->bus_info, pci_name(bp->pdev), sizeof(info->bus_info));
+7 -7
drivers/net/ethernet/broadcom/bnx2x/bnx2x_link.c
··· 6163 6163 6164 6164 static int bnx2x_null_format_ver(u32 spirom_ver, u8 *str, u16 *len) 6165 6165 { 6166 - str[0] = '\0'; 6167 - (*len)--; 6166 + if (*len) 6167 + str[0] = '\0'; 6168 6168 return 0; 6169 6169 } 6170 6170 ··· 6173 6173 u16 ret; 6174 6174 6175 6175 if (*len < 10) { 6176 - /* Need more than 10chars for this format */ 6176 + /* Need more than 10 chars for this format */ 6177 6177 bnx2x_null_format_ver(num, str, len); 6178 6178 return -EINVAL; 6179 6179 } ··· 6188 6188 { 6189 6189 u16 ret; 6190 6190 6191 - if (*len < 10) { 6192 - /* Need more than 10chars for this format */ 6191 + if (*len < 9) { 6192 + /* Need more than 9 chars for this format */ 6193 6193 bnx2x_null_format_ver(num, str, len); 6194 6194 return -EINVAL; 6195 6195 } ··· 6208 6208 int status = 0; 6209 6209 u8 *ver_p = version; 6210 6210 u16 remain_len = len; 6211 - if (version == NULL || params == NULL) 6211 + if (version == NULL || params == NULL || len == 0) 6212 6212 return -EINVAL; 6213 6213 bp = params->bp; 6214 6214 ··· 11546 11546 str[2] = (spirom_ver & 0xFF0000) >> 16; 11547 11547 str[3] = (spirom_ver & 0xFF000000) >> 24; 11548 11548 str[4] = '\0'; 11549 - *len -= 5; 11549 + *len -= 4; 11550 11550 return 0; 11551 11551 } 11552 11552