From: Niklas Cassel <Niklas.Cassel@wdc.com>
To: Kees Cook <keescook@chromium.org>
Cc: Michael Chan <michael.chan@broadcom.com>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-hardening@vger.kernel.org"
<linux-hardening@vger.kernel.org>
Subject: Re: [PATCH] bnxt: Do not read past the end of test names
Date: Thu, 19 Jan 2023 10:00:55 +0000 [thread overview]
Message-ID: <Y8kU10ZFNHJ2Q1IR@x1-carbon> (raw)
In-Reply-To: <20230118203457.never.612-kees@kernel.org>
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>
next prev parent reply other threads:[~2023-01-19 10:01 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-18 20:35 [PATCH] bnxt: Do not read past the end of test names Kees Cook
2023-01-18 20:59 ` Michael Chan
2023-01-19 10:00 ` Niklas Cassel [this message]
2023-01-20 13:00 ` patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y8kU10ZFNHJ2Q1IR@x1-carbon \
--to=niklas.cassel@wdc.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=keescook@chromium.org \
--cc=kuba@kernel.org \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=michael.chan@broadcom.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.