Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 13 Jan 2023 14:44:32 -0800
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
Message-ID: <202301131415.6E0C3BF328@keescook>
References: <20220920192202.190793-1-keescook@chromium.org>
 <20220920192202.190793-5-keescook@chromium.org>
 <Y8F/1w1AZTvLglFX@x1-carbon>
 <Y8GB9DMtcSP/8e/i@x1-carbon>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y8GB9DMtcSP/8e/i@x1-carbon>
Precedence: bulk

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