lkml.org 
[lkml]   [2023]   [Jan]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH] bnxt: Do not read past the end of test names
    Date
    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>
    \
     
     \ /
      Last update: 2023-03-26 23:47    [W:4.702 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site