[PATCH] bnxt: Do not read past the end of test names

[PATCH] bnxt: Do not read past the end of test names

[Kees Cook]

Test names were being concatenated based on a offset beyond the end of
the first name, which tripped the buffer overflow detection logic:

 detected buffer overflow in strnlen
 [...]
 Call Trace:
 bnxt_ethtool_init.cold+0x18/0x18

Refactor struct hwrm_selftest_qlist_output to use an actual array,
and adjust the concatenation to use snprintf() rather than a series of
strncat() calls.

Reported-by: Niklas Cassel <Niklas.Cassel@wdc.com>
Link: https://lore.kernel.org/lkml/Y8F%2F1w1AZTvLglFX@x1-carbon/
Tested-by: Niklas Cassel <Niklas.Cassel@wdc.com>
Fixes: eb51365846bc ("bnxt_en: Add basic ethtool -t selftest support.")
Cc: Michael Chan <michael.chan@broadcom.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 13 ++++---------
 drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h     |  9 +--------
 2 files changed, 5 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index cbf17fcfb7ab..ec573127b707 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -3969,7 +3969,7 @@ void bnxt_ethtool_init(struct bnxt *bp)
 		test_info->timeout = HWRM_CMD_TIMEOUT;
 	for (i = 0; i < bp->num_tests; i++) {
 		char *str = test_info->string[i];
-		char *fw_str = resp->test0_name + i * 32;
+		char *fw_str = resp->test_name[i];
 
 		if (i == BNXT_MACLPBK_TEST_IDX) {
 			strcpy(str, "Mac loopback test (offline)");
@@ -3980,14 +3980,9 @@ void bnxt_ethtool_init(struct bnxt *bp)
 		} else if (i == BNXT_IRQ_TEST_IDX) {
 			strcpy(str, "Interrupt_test (offline)");
 		} else {
-			strscpy(str, fw_str, ETH_GSTRING_LEN);
-			strncat(str, " test", ETH_GSTRING_LEN - strlen(str));
-			if (test_info->offline_mask & (1 << i))
-				strncat(str, " (offline)",
-					ETH_GSTRING_LEN - strlen(str));
-			else
-				strncat(str, " (online)",
-					ETH_GSTRING_LEN - strlen(str));
+			snprintf(str, ETH_GSTRING_LEN, "%s test (%s)",
+				 fw_str, test_info->offline_mask & (1 << i) ?
+					"offline" : "online");
 		}
 	}
 
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
index 2686a714a59f..a5408879e077 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
@@ -10249,14 +10249,7 @@ struct hwrm_selftest_qlist_output {
 	u8	unused_0;
 	__le16	test_timeout;
 	u8	unused_1[2];
-	char	test0_name[32];
-	char	test1_name[32];
-	char	test2_name[32];
-	char	test3_name[32];
-	char	test4_name[32];
-	char	test5_name[32];
-	char	test6_name[32];
-	char	test7_name[32];
+	char	test_name[8][32];
 	u8	eyescope_target_BER_support;
 	#define SELFTEST_QLIST_RESP_EYESCOPE_TARGET_BER_SUPPORT_BER_1E8_SUPPORTED  0x0UL
 	#define SELFTEST_QLIST_RESP_EYESCOPE_TARGET_BER_SUPPORT_BER_1E9_SUPPORTED  0x1UL
-- 
2.34.1

Re: [PATCH] bnxt: Do not read past the end of test names

[Michael Chan]

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

On Wed, Jan 18, 2023 at 12:35 PM Kees Cook <keescook@chromium.org> wrote:
>
> Test names were being concatenated based on a offset beyond the end of
> the first name, which tripped the buffer overflow detection logic:
>
>  detected buffer overflow in strnlen
>  [...]
>  Call Trace:
>  bnxt_ethtool_init.cold+0x18/0x18
>
> Refactor struct hwrm_selftest_qlist_output to use an actual array,
> and adjust the concatenation to use snprintf() rather than a series of
> strncat() calls.
>
> Reported-by: Niklas Cassel <Niklas.Cassel@wdc.com>
> Link: https://lore.kernel.org/lkml/Y8F%2F1w1AZTvLglFX@x1-carbon/
> Tested-by: Niklas Cassel <Niklas.Cassel@wdc.com>
> Fixes: eb51365846bc ("bnxt_en: Add basic ethtool -t selftest support.")
> Cc: Michael Chan <michael.chan@broadcom.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>

Reviewed-by: Michael Chan <michael.chan@broadcom.com>

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
> index 2686a714a59f..a5408879e077 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
> @@ -10249,14 +10249,7 @@ struct hwrm_selftest_qlist_output {
>         u8      unused_0;
>         __le16  test_timeout;
>         u8      unused_1[2];
> -       char    test0_name[32];
> -       char    test1_name[32];
> -       char    test2_name[32];
> -       char    test3_name[32];
> -       char    test4_name[32];
> -       char    test5_name[32];
> -       char    test6_name[32];
> -       char    test7_name[32];
> +       char    test_name[8][32];

bnxt_hsi.h is a generated file.  I will need to make a request so that
future versions will be generated like this.

Thanks.

>         u8      eyescope_target_BER_support;
>         #define SELFTEST_QLIST_RESP_EYESCOPE_TARGET_BER_SUPPORT_BER_1E8_SUPPORTED  0x0UL
>         #define SELFTEST_QLIST_RESP_EYESCOPE_TARGET_BER_SUPPORT_BER_1E9_SUPPORTED  0x1UL
> --
> 2.34.1
>

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

Re: [PATCH] bnxt: Do not read past the end of test names

[Niklas Cassel]

On Wed, Jan 18, 2023 at 12:35:01PM -0800, Kees Cook wrote:
> Test names were being concatenated based on a offset beyond the end of
> the first name, which tripped the buffer overflow detection logic:
> 
>  detected buffer overflow in strnlen
>  [...]
>  Call Trace:
>  bnxt_ethtool_init.cold+0x18/0x18
> 
> Refactor struct hwrm_selftest_qlist_output to use an actual array,
> and adjust the concatenation to use snprintf() rather than a series of
> strncat() calls.
> 
> Reported-by: Niklas Cassel <Niklas.Cassel@wdc.com>
> Link: https://lore.kernel.org/lkml/Y8F%2F1w1AZTvLglFX@x1-carbon/
> Tested-by: Niklas Cassel <Niklas.Cassel@wdc.com>
> Fixes: eb51365846bc ("bnxt_en: Add basic ethtool -t selftest support.")
> Cc: Michael Chan <michael.chan@broadcom.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 13 ++++---------
>  drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h     |  9 +--------
>  2 files changed, 5 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index cbf17fcfb7ab..ec573127b707 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -3969,7 +3969,7 @@ void bnxt_ethtool_init(struct bnxt *bp)
>  		test_info->timeout = HWRM_CMD_TIMEOUT;
>  	for (i = 0; i < bp->num_tests; i++) {
>  		char *str = test_info->string[i];
> -		char *fw_str = resp->test0_name + i * 32;
> +		char *fw_str = resp->test_name[i];
>  
>  		if (i == BNXT_MACLPBK_TEST_IDX) {
>  			strcpy(str, "Mac loopback test (offline)");
> @@ -3980,14 +3980,9 @@ void bnxt_ethtool_init(struct bnxt *bp)
>  		} else if (i == BNXT_IRQ_TEST_IDX) {
>  			strcpy(str, "Interrupt_test (offline)");
>  		} else {
> -			strscpy(str, fw_str, ETH_GSTRING_LEN);
> -			strncat(str, " test", ETH_GSTRING_LEN - strlen(str));
> -			if (test_info->offline_mask & (1 << i))
> -				strncat(str, " (offline)",
> -					ETH_GSTRING_LEN - strlen(str));
> -			else
> -				strncat(str, " (online)",
> -					ETH_GSTRING_LEN - strlen(str));
> +			snprintf(str, ETH_GSTRING_LEN, "%s test (%s)",
> +				 fw_str, test_info->offline_mask & (1 << i) ?
> +					"offline" : "online");
>  		}
>  	}
>  
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
> index 2686a714a59f..a5408879e077 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
> @@ -10249,14 +10249,7 @@ struct hwrm_selftest_qlist_output {
>  	u8	unused_0;
>  	__le16	test_timeout;
>  	u8	unused_1[2];
> -	char	test0_name[32];
> -	char	test1_name[32];
> -	char	test2_name[32];
> -	char	test3_name[32];
> -	char	test4_name[32];
> -	char	test5_name[32];
> -	char	test6_name[32];
> -	char	test7_name[32];
> +	char	test_name[8][32];
>  	u8	eyescope_target_BER_support;
>  	#define SELFTEST_QLIST_RESP_EYESCOPE_TARGET_BER_SUPPORT_BER_1E8_SUPPORTED  0x0UL
>  	#define SELFTEST_QLIST_RESP_EYESCOPE_TARGET_BER_SUPPORT_BER_1E9_SUPPORTED  0x1UL
> -- 
> 2.34.1
> 

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

Re: [PATCH] bnxt: Do not read past the end of test names

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Wed, 18 Jan 2023 12:35:01 -0800 you wrote:
> Test names were being concatenated based on a offset beyond the end of
> the first name, which tripped the buffer overflow detection logic:
> 
>  detected buffer overflow in strnlen
>  [...]
>  Call Trace:
>  bnxt_ethtool_init.cold+0x18/0x18
> 
> [...]

Here is the summary with links:
  - bnxt: Do not read past the end of test names
    https://git.kernel.org/netdev/net/c/d3e599c090fc

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html