* linux-next - bxnt buffer overflow in strnlen [not found] ` <20220920192202.190793-5-keescook@chromium.org> @ 2023-01-13 15:59 ` Niklas Cassel 2023-01-13 16:08 ` linux-next - bnxt " Niklas Cassel 0 siblings, 1 reply; 4+ messages in thread From: Niklas Cassel @ 2023-01-13 15:59 UTC (permalink / raw) To: Kees Cook Cc: linux-hardening@vger.kernel.org, Miguel Ojeda, Siddhesh Poyarekar, Arnd Bergmann, Nick Desaulniers, Nathan Chancellor, Tom Rix, llvm@lists.linux.dev, Juergen Gross, Boris Ostrovsky, linux-kernel@vger.kernel.org, Damien Le Moal, linux-next@vger.kernel.org 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. > > Additionally update the VMALLOC_LINEAR_OVERFLOW LKDTM test to make the > hint invisible, otherwise the architectural defense is not exercised > (the buffer overflow is detected in the memset() rather than when it > crosses the edge of the allocation). > > Cc: Miguel Ojeda <ojeda@kernel.org> > Cc: Siddhesh Poyarekar <siddhesh@gotplt.org> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Nick Desaulniers <ndesaulniers@google.com> > Cc: Nathan Chancellor <nathan@kernel.org> > Cc: Tom Rix <trix@redhat.com> > Cc: linux-hardening@vger.kernel.org > Cc: llvm@lists.linux.dev > Signed-off-by: Kees Cook <keescook@chromium.org> > --- Hello Kees, Unfortunately, this commit introduces a crash in the bxnt ethernet driver when booting linux-next. I haven't looked at the code in the bxnt 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.810683] ------------[ cut here ]------------ <2>[ 10.816035] kernel BUG at lib/string_helpers.c:1027! <4>[ 10.821753] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI <4>[ 10.822737] CPU: 3 PID: 1 Comm: swapper/0 Not tainted 6.2.0-rc3-next-20230112+ #4 <4>[ 10.834787] Hardware name: Supermicro Super Server/H12SSL-NT, BIOS 2.4 04/14/2022 <4>[ 10.839875] RIP: 0010:fortify_panic+0xf/0x11 <4>[ 10.844962] Code: e0 e8 da 83 0a ff e9 31 45 6d ff 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 48 89 fe 48 c7 c7 78 69 d6 9f e8 01 ea fc ff <0f> 0b 48 8b 4c 24 18 48 8b 54 24 10 4c 8d 44 24 25 48 c7 c7 b6 69 <4>[ 10.865321] RSP: 0018:ffffb547c005bb98 EFLAGS: 00010246 <4>[ 10.870406] RAX: 0000000000000023 RBX: ffff94f0582bc400 RCX: 0000000000000000 <4>[ 10.880584] RDX: 0000000000000001 RSI: 00000000ffffdfff RDI: 00000000ffffffff <4>[ 10.885672] RBP: ffff94f0582bc424 R08: 0000000000000000 R09: ffffb547c005ba60 <4>[ 10.895849] R10: 0000000000000003 R11: ffffffffa0182448 R12: 696c66666f282074 <4>[ 10.900937] R13: 736574206b636162 R14: 0000000000000001 R15: ffff94f0545f8b40 <4>[ 10.911113] FS: 0000000000000000(0000) GS:ffff950f07380000(0000) knlGS:0000000000000000 <4>[ 10.916201] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 <4>[ 10.926382] CR2: 0000000000000000 CR3: 000000204c05a000 CR4: 0000000000350ee0 <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 <4>[ 10.936745] ? dma_pool_free+0x14d/0x160 <6>[ 10.942591] ata10: SATA link down (SStatus 0 SControl 300) <4>[ 10.942663] bnxt_fw_init_one_p2+0x18d/0x5e0 <6>[ 10.949046] ata4: SATA link down (SStatus 0 SControl 300) <4>[ 10.949841] bnxt_init_one+0x401/0xf10 <6>[ 10.958451] ata6: SATA link down (SStatus 0 SControl 300) <4>[ 10.958854] local_pci_probe+0x41/0x80 <6>[ 10.968114] ata3: SATA link down (SStatus 0 SControl 300) <4>[ 10.968892] pci_device_probe+0x1e2/0x210 <6>[ 10.977259] ata8: SATA link down (SStatus 0 SControl 300) <4>[ 10.977657] really_probe+0xde/0x380 <6>[ 10.986406] ata5: SATA link down (SStatus 0 SControl 300) <4>[ 10.986817] ? pm_runtime_barrier+0x50/0x90 <4>[ 10.986817] __driver_probe_device+0x78/0x170 <6>[ 10.996042] ata7: SATA link down (SStatus 0 SControl 300) <4>[ 10.996978] driver_probe_device+0x1f/0x90 <4>[ 10.996978] __driver_attach+0xd2/0x1c0 <4>[ 10.996978] ? __pfx___driver_attach+0x10/0x10 <4>[ 10.996978] bus_for_each_dev+0x65/0x90 <4>[ 11.047368] bus_add_driver+0x1b1/0x200 <4>[ 11.052640] driver_register+0x89/0xe0 <4>[ 11.057487] ? __pfx_bnxt_init+0x10/0x10 <4>[ 11.061634] bnxt_init+0x20/0x33 <4>[ 11.065015] do_one_initcall+0x5b/0x340 <4>[ 11.070105] ? rcu_read_lock_sched_held+0x3f/0x80 <4>[ 11.075198] kernel_init_freeable+0x29e/0x2ee <4>[ 11.080635] ? __pfx_kernel_init+0x10/0x10 <4>[ 11.085379] kernel_init+0x16/0x140 <6>[ 11.087743] ata16: SATA link down (SStatus 0 SControl 300) <4>[ 11.086528] ret_from_fork+0x2c/0x50 <4>[ 11.086528] </TASK> <6>[ 11.094437] ata17: SATA link down (SStatus 0 SControl 300) <4>[ 11.094645] Modules linked in: <4>[ 11.097999] ---[ end trace 0000000000000000 ]--- <6>[ 11.100194] ata18: SATA link down (SStatus 0 SControl 300) Kind regards, Niklas ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: linux-next - bnxt buffer overflow in strnlen 2023-01-13 15:59 ` linux-next - bxnt buffer overflow in strnlen Niklas Cassel @ 2023-01-13 16:08 ` Niklas Cassel 2023-01-13 22:44 ` Kees Cook 0 siblings, 1 reply; 4+ messages in thread From: Niklas Cassel @ 2023-01-13 16:08 UTC (permalink / raw) To: Kees Cook Cc: linux-hardening@vger.kernel.org, Miguel Ojeda, Siddhesh Poyarekar, Arnd Bergmann, Nick Desaulniers, Nathan Chancellor, Tom Rix, llvm@lists.linux.dev, Juergen Gross, Boris Ostrovsky, linux-kernel@vger.kernel.org, Damien Le Moal, linux-next@vger.kernel.org, netdev@vger.kernel.org, Michael Chan 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. > > > > Additionally update the VMALLOC_LINEAR_OVERFLOW LKDTM test to make the > > hint invisible, otherwise the architectural defense is not exercised > > (the buffer overflow is detected in the memset() rather than when it > > crosses the edge of the allocation). > > > > Cc: Miguel Ojeda <ojeda@kernel.org> > > Cc: Siddhesh Poyarekar <siddhesh@gotplt.org> > > Cc: Arnd Bergmann <arnd@arndb.de> > > Cc: Nick Desaulniers <ndesaulniers@google.com> > > Cc: Nathan Chancellor <nathan@kernel.org> > > Cc: Tom Rix <trix@redhat.com> > > Cc: linux-hardening@vger.kernel.org > > Cc: llvm@lists.linux.dev > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > Hello Kees, > > Unfortunately, this commit introduces a crash in the bnxt > ethernet driver when booting linux-next. > > 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.810683] ------------[ cut here ]------------ > <2>[ 10.816035] kernel BUG at lib/string_helpers.c:1027! > <4>[ 10.821753] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI > <4>[ 10.822737] CPU: 3 PID: 1 Comm: swapper/0 Not tainted 6.2.0-rc3-next-20230112+ #4 > <4>[ 10.834787] Hardware name: Supermicro Super Server/H12SSL-NT, BIOS 2.4 04/14/2022 > <4>[ 10.839875] RIP: 0010:fortify_panic+0xf/0x11 > <4>[ 10.844962] Code: e0 e8 da 83 0a ff e9 31 45 6d ff 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 48 89 fe 48 c7 c7 78 69 d6 9f e8 01 ea fc ff <0f> 0b 48 8b 4c 24 18 48 8b 54 24 10 4c 8d 44 24 25 48 c7 c7 b6 69 > <4>[ 10.865321] RSP: 0018:ffffb547c005bb98 EFLAGS: 00010246 > <4>[ 10.870406] RAX: 0000000000000023 RBX: ffff94f0582bc400 RCX: 0000000000000000 > <4>[ 10.880584] RDX: 0000000000000001 RSI: 00000000ffffdfff RDI: 00000000ffffffff > <4>[ 10.885672] RBP: ffff94f0582bc424 R08: 0000000000000000 R09: ffffb547c005ba60 > <4>[ 10.895849] R10: 0000000000000003 R11: ffffffffa0182448 R12: 696c66666f282074 > <4>[ 10.900937] R13: 736574206b636162 R14: 0000000000000001 R15: ffff94f0545f8b40 > <4>[ 10.911113] FS: 0000000000000000(0000) GS:ffff950f07380000(0000) knlGS:0000000000000000 > <4>[ 10.916201] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > <4>[ 10.926382] CR2: 0000000000000000 CR3: 000000204c05a000 CR4: 0000000000350ee0 > <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 > <4>[ 10.936745] ? dma_pool_free+0x14d/0x160 > <6>[ 10.942591] ata10: SATA link down (SStatus 0 SControl 300) > <4>[ 10.942663] bnxt_fw_init_one_p2+0x18d/0x5e0 > <6>[ 10.949046] ata4: SATA link down (SStatus 0 SControl 300) > <4>[ 10.949841] bnxt_init_one+0x401/0xf10 > <6>[ 10.958451] ata6: SATA link down (SStatus 0 SControl 300) > <4>[ 10.958854] local_pci_probe+0x41/0x80 > <6>[ 10.968114] ata3: SATA link down (SStatus 0 SControl 300) > <4>[ 10.968892] pci_device_probe+0x1e2/0x210 > <6>[ 10.977259] ata8: SATA link down (SStatus 0 SControl 300) > <4>[ 10.977657] really_probe+0xde/0x380 > <6>[ 10.986406] ata5: SATA link down (SStatus 0 SControl 300) > <4>[ 10.986817] ? pm_runtime_barrier+0x50/0x90 > <4>[ 10.986817] __driver_probe_device+0x78/0x170 > <6>[ 10.996042] ata7: SATA link down (SStatus 0 SControl 300) > <4>[ 10.996978] driver_probe_device+0x1f/0x90 > <4>[ 10.996978] __driver_attach+0xd2/0x1c0 > <4>[ 10.996978] ? __pfx___driver_attach+0x10/0x10 > <4>[ 10.996978] bus_for_each_dev+0x65/0x90 > <4>[ 11.047368] bus_add_driver+0x1b1/0x200 > <4>[ 11.052640] driver_register+0x89/0xe0 > <4>[ 11.057487] ? __pfx_bnxt_init+0x10/0x10 > <4>[ 11.061634] bnxt_init+0x20/0x33 > <4>[ 11.065015] do_one_initcall+0x5b/0x340 > <4>[ 11.070105] ? rcu_read_lock_sched_held+0x3f/0x80 > <4>[ 11.075198] kernel_init_freeable+0x29e/0x2ee > <4>[ 11.080635] ? __pfx_kernel_init+0x10/0x10 > <4>[ 11.085379] kernel_init+0x16/0x140 > <6>[ 11.087743] ata16: SATA link down (SStatus 0 SControl 300) > <4>[ 11.086528] ret_from_fork+0x2c/0x50 > <4>[ 11.086528] </TASK> > <6>[ 11.094437] ata17: SATA link down (SStatus 0 SControl 300) > <4>[ 11.094645] Modules linked in: > <4>[ 11.097999] ---[ end trace 0000000000000000 ]--- > <6>[ 11.100194] ata18: SATA link down (SStatus 0 SControl 300) > > > Kind regards, > Niklas +netdev +bnxt maintainers ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: linux-next - bnxt buffer overflow in strnlen 2023-01-13 16:08 ` linux-next - bnxt " Niklas Cassel @ 2023-01-13 22:44 ` Kees Cook 2023-01-16 10:56 ` Niklas Cassel 0 siblings, 1 reply; 4+ messages in thread From: Kees Cook @ 2023-01-13 22:44 UTC (permalink / raw) To: Niklas Cassel Cc: linux-hardening@vger.kernel.org, Miguel Ojeda, Siddhesh Poyarekar, Arnd Bergmann, Nick Desaulniers, Nathan Chancellor, Tom Rix, llvm@lists.linux.dev, Juergen Gross, Boris Ostrovsky, linux-kernel@vger.kernel.org, Damien Le Moal, linux-next@vger.kernel.org, netdev@vger.kernel.org, Michael Chan 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: linux-next - bnxt buffer overflow in strnlen 2023-01-13 22:44 ` Kees Cook @ 2023-01-16 10:56 ` Niklas Cassel 0 siblings, 0 replies; 4+ messages in thread From: Niklas Cassel @ 2023-01-16 10:56 UTC (permalink / raw) To: Kees Cook Cc: linux-hardening@vger.kernel.org, Miguel Ojeda, Siddhesh Poyarekar, Arnd Bergmann, Nick Desaulniers, Nathan Chancellor, Tom Rix, llvm@lists.linux.dev, Juergen Gross, Boris Ostrovsky, linux-kernel@vger.kernel.org, Damien Le Moal, linux-next@vger.kernel.org, netdev@vger.kernel.org, Michael Chan On Fri, Jan 13, 2023 at 02:44:32PM -0800, Kees Cook wrote: > On Fri, Jan 13, 2023 at 04:08:21PM +0000, Niklas Cassel wrote: > > > Hello Kees, > > > > > > Unfortunately, this commit introduces a crash in the bnxt > > > ethernet driver when booting linux-next. (snip) > > 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? Yes, it does! Thank you very much Kees, both for this patch, and for all the excellent work that you've done with regard to kernel hardening in general over the years. Feel free to add my: Tested-by: Niklas Cassel <niklas.cassel@wdc.com> if you send out a real patch. Kind regards, Niklas > > > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-01-16 10:57 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20220920192202.190793-1-keescook@chromium.org>
[not found] ` <20220920192202.190793-5-keescook@chromium.org>
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
2023-01-16 10:56 ` Niklas Cassel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox