From: Kees Cook <keescook@chromium.org>
To: Niklas Cassel <Niklas.Cassel@wdc.com>
Cc: "linux-hardening@vger.kernel.org"
<linux-hardening@vger.kernel.org>,
Miguel Ojeda <ojeda@kernel.org>,
Siddhesh Poyarekar <siddhesh@gotplt.org>,
Arnd Bergmann <arnd@arndb.de>,
Nick Desaulniers <ndesaulniers@google.com>,
Nathan Chancellor <nathan@kernel.org>, Tom Rix <trix@redhat.com>,
"llvm@lists.linux.dev" <llvm@lists.linux.dev>,
Juergen Gross <jgross@suse.com>,
Boris Ostrovsky <boris.ostrovsky@oracle.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Damien Le Moal <damien.lemoal@opensource.wdc.com>,
"linux-next@vger.kernel.org" <linux-next@vger.kernel.org>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
Michael Chan <michael.chan@broadcom.com>
Subject: Re: linux-next - bnxt buffer overflow in strnlen
Date: Fri, 13 Jan 2023 14:44:32 -0800 [thread overview]
Message-ID: <202301131415.6E0C3BF328@keescook> (raw)
In-Reply-To: <Y8GB9DMtcSP/8e/i@x1-carbon>
On Fri, Jan 13, 2023 at 04:08:21PM +0000, Niklas Cassel wrote:
> On Fri, Jan 13, 2023 at 04:59:19PM +0100, Niklas Cassel wrote:
> > On Tue, Sep 20, 2022 at 12:22:02PM -0700, Kees Cook wrote:
> > > Since the commits starting with c37495d6254c ("slab: add __alloc_size
> > > attributes for better bounds checking"), the compilers have runtime
> > > allocation size hints available in some places. This was immediately
> > > available to CONFIG_UBSAN_BOUNDS, but CONFIG_FORTIFY_SOURCE needed
> > > updating to explicitly make use the hints via the associated
> > > __builtin_dynamic_object_size() helper. Detect and use the builtin when
> > > it is available, increasing the accuracy of the mitigation. When runtime
> > > sizes are not available, __builtin_dynamic_object_size() falls back to
> > > __builtin_object_size(), leaving the existing bounds checking unchanged.
> > > [...]
> > Hello Kees,
> >
> > Unfortunately, this commit introduces a crash in the bnxt
> > ethernet driver when booting linux-next.
Hi! Thanks for the report. Notes below...
> > I haven't looked at the code in the bnxt ethernet driver,
> > I simply know that machine boots fine on v6.2.0-rc3,
> > but fails to boot with linux-next.
> >
> > So I started an automatic git bisect, which returned:
> > 439a1bcac648 ("fortify: Use __builtin_dynamic_object_size() when available")
> >
> > $ grep CC_VERSION .config
> > CONFIG_CC_VERSION_TEXT="gcc (GCC) 12.2.1 20221121 (Red Hat 12.2.1-4)"
> > CONFIG_GCC_VERSION=120201
> >
> > $ grep FORTIFY .config
> > CONFIG_ARCH_HAS_FORTIFY_SOURCE=y
> > CONFIG_FORTIFY_SOURCE=y
> >
> >
> > dmesg output:
> >
> > <0>[ 10.805253] detected buffer overflow in strnlen
> [...]
> > <4>[ 10.931470] Call Trace:
> > <6>[ 10.936317] ata9: SATA link down (SStatus 0 SControl 300)
> > <4>[ 10.936745] <TASK>
> > <4>[ 10.936745] bnxt_ethtool_init.cold+0x18/0x18
Are you able to run:
$ ./scripts/faddr2line vmlinux bnxt_ethtool_init.cold+0x18/0x18
to find the exact line it's failing on, just to be sure we're looking in
the right place?
There are a bunch of string functions being used in a loop
bnxt_ethtool_init(). Here's the code:
if (bp->num_tests > BNXT_MAX_TEST)
bp->num_tests = BNXT_MAX_TEST;
...
for (i = 0; i < bp->num_tests; i++) {
char *str = test_info->string[i];
char *fw_str = resp->test0_name + i * 32;
if (i == BNXT_MACLPBK_TEST_IDX) {
strcpy(str, "Mac loopback test (offline)");
} else if (i == BNXT_PHYLPBK_TEST_IDX) {
strcpy(str, "Phy loopback test (offline)");
} else if (i == BNXT_EXTLPBK_TEST_IDX) {
strcpy(str, "Ext loopback test (offline)");
} 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));
}
}
The hardened strnlen() is used internally to the hardened strcpy() and
strscpy()'s source argument, and strncat()'s dest and source arguments.
The only non-literal source argument is fw_str.
The destination in this loop is always "str", which is test_info->string[i].
I'd expect "str" to always be processed as fixed size:
struct bnxt_test_info {
u8 offline_mask;
u16 timeout;
char string[BNXT_MAX_TEST][ETH_GSTRING_LEN];
};
#define ETH_GSTRING_LEN 32
#define BNXT_MAX_TEST 8
And the allocation matches that size:
test_info = kzalloc(sizeof(*bp->test_info), GFP_KERNEL);
(bp->test_info is, indeed struct bnxt_test_info too.)
The loop cannot reach BNXT_MAX_TEST. It looks like fw_str's size isn't
known dynamically, so that shouldn't be a change. (It's assigned from
a void * return.) So I suspect "str" ran off the end of the allocation,
which implies that "fw_str" must be >= ETH_GSTRING_LEN. This line looks
very suspicious:
char *fw_str = resp->test0_name + i * 32;
I also note that the return value of strscpy() is not checked...
Let's see...
struct hwrm_selftest_qlist_output {
...
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];
...
};
Ew. So, yes, it's specifically reach past the end of the test0_name[]
array, *and* is may overflow the heap. Does this patch solve it for you?
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
--
Kees Cook
next prev parent reply other threads:[~2023-01-13 22:44 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-20 19:21 [PATCH 0/4] fortify: Use __builtin_dynamic_object_size() when available Kees Cook
2022-09-20 19:21 ` [PATCH 1/4] x86/entry: Work around Clang __bdos() bug Kees Cook
2022-09-21 0:07 ` Boris Ostrovsky
2022-09-20 19:22 ` [PATCH 2/4] fortify: Explicitly check bounds are compile-time constants Kees Cook
2022-09-21 11:48 ` Siddhesh Poyarekar
2022-09-22 3:46 ` Kees Cook
2022-09-20 19:22 ` [PATCH 3/4] fortify: Convert to struct vs member helpers Kees Cook
2022-09-20 19:22 ` [PATCH 4/4] fortify: Use __builtin_dynamic_object_size() when available Kees Cook
2022-09-21 11:24 ` Miguel Ojeda
2022-09-21 11:43 ` Siddhesh Poyarekar
2022-09-22 3:33 ` Kees Cook
2022-09-22 14:45 ` Siddhesh Poyarekar
2022-11-22 10:20 ` Siddhesh Poyarekar
2022-11-23 5:15 ` Kees Cook
2022-11-23 15:29 ` Siddhesh Poyarekar
2023-01-13 15:59 ` linux-next - bxnt buffer overflow in strnlen Niklas Cassel
2023-01-13 16:08 ` linux-next - bnxt " Niklas Cassel
2023-01-13 22:44 ` Kees Cook [this message]
2023-01-16 10:56 ` Niklas Cassel
2022-09-22 20:26 ` [PATCH 0/4] fortify: Use __builtin_dynamic_object_size() when available Siddhesh Poyarekar
2022-09-23 0:20 ` Kees Cook
2022-09-23 0:55 ` Siddhesh Poyarekar
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=202301131415.6E0C3BF328@keescook \
--to=keescook@chromium.org \
--cc=Niklas.Cassel@wdc.com \
--cc=arnd@arndb.de \
--cc=boris.ostrovsky@oracle.com \
--cc=damien.lemoal@opensource.wdc.com \
--cc=jgross@suse.com \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-next@vger.kernel.org \
--cc=llvm@lists.linux.dev \
--cc=michael.chan@broadcom.com \
--cc=nathan@kernel.org \
--cc=ndesaulniers@google.com \
--cc=netdev@vger.kernel.org \
--cc=ojeda@kernel.org \
--cc=siddhesh@gotplt.org \
--cc=trix@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).