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 <[email protected]>
Cc: Siddhesh Poyarekar <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Nick Desaulniers <[email protected]>
Cc: Nathan Chancellor <[email protected]>
Cc: Tom Rix <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
drivers/misc/lkdtm/heap.c | 1 +
include/linux/compiler_attributes.h | 5 +++++
include/linux/fortify-string.h | 7 +++++++
3 files changed, 13 insertions(+)
diff --git a/drivers/misc/lkdtm/heap.c b/drivers/misc/lkdtm/heap.c
index 62516078a619..0ce4cbf6abda 100644
--- a/drivers/misc/lkdtm/heap.c
+++ b/drivers/misc/lkdtm/heap.c
@@ -31,6 +31,7 @@ static void lkdtm_VMALLOC_LINEAR_OVERFLOW(void)
char *one, *two;
one = vzalloc(PAGE_SIZE);
+ OPTIMIZER_HIDE_VAR(one);
two = vzalloc(PAGE_SIZE);
pr_info("Attempting vmalloc linear overflow ...\n");
diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
index 445e80517cab..9a9907fad6fd 100644
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -296,6 +296,11 @@
*
* clang: https://clang.llvm.org/docs/AttributeReference.html#pass-object-size-pass-dynamic-object-size
*/
+#if __has_attribute(__pass_dynamic_object_size__)
+# define __pass_dynamic_object_size(type) __attribute__((__pass_dynamic_object_size__(type)))
+#else
+# define __pass_dynamic_object_size(type)
+#endif
#if __has_attribute(__pass_object_size__)
# define __pass_object_size(type) __attribute__((__pass_object_size__(type)))
#else
diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index 3f1178584d7b..dd7f85d74ade 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -77,10 +77,17 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size)
* size, rather than struct size), but there remain some stragglers using
* type 0 that will be converted in the future.
*/
+#if __has_builtin(__builtin_dynamic_object_size)
+#define POS __pass_dynamic_object_size(1)
+#define POS0 __pass_dynamic_object_size(0)
+#define __struct_size(p) __builtin_dynamic_object_size(p, 0)
+#define __member_size(p) __builtin_dynamic_object_size(p, 1)
+#else
#define POS __pass_object_size(1)
#define POS0 __pass_object_size(0)
#define __struct_size(p) __builtin_object_size(p, 0)
#define __member_size(p) __builtin_object_size(p, 1)
+#endif
#define __compiletime_lessthan(bounds, length) ( \
__builtin_constant_p(length) && \
--
2.34.1
On 2022-09-20 15:22, 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.
I don't know yet what the overhead is for __builtin_dynamic_object_size
vs __builtin_object_size, were you able to measure it somehow for the
kernel? If there's a significant tradeoff, it may make sense to provide
a user override.
Thanks,
Sid
On Tue, Sep 20, 2022 at 9:22 PM Kees Cook <[email protected]> wrote:
>
> include/linux/compiler_attributes.h | 5 +++++
Reviewed-by: Miguel Ojeda <[email protected]>
Cheers,
Miguel
On Wed, Sep 21, 2022 at 07:43:17AM -0400, Siddhesh Poyarekar wrote:
> On 2022-09-20 15:22, 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.
>
> I don't know yet what the overhead is for __builtin_dynamic_object_size vs
> __builtin_object_size, were you able to measure it somehow for the kernel?
> If there's a significant tradeoff, it may make sense to provide a user
> override.
So far I've not seen any measurable performance difference, but I just
may not be creative enough yet.
So far, the tunable is building a kernel with or without FORTIFY_SOURCE
and UBSAN_BOUNDS. :)
--
Kees Cook
On 2022-09-21 23:33, Kees Cook wrote:
> On Wed, Sep 21, 2022 at 07:43:17AM -0400, Siddhesh Poyarekar wrote:
>> On 2022-09-20 15:22, 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.
>>
>> I don't know yet what the overhead is for __builtin_dynamic_object_size vs
>> __builtin_object_size, were you able to measure it somehow for the kernel?
>> If there's a significant tradeoff, it may make sense to provide a user
>> override.
>
> So far I've not seen any measurable performance difference, but I just
> may not be creative enough yet.
>
> So far, the tunable is building a kernel with or without FORTIFY_SOURCE
> and UBSAN_BOUNDS. :)
>
The overhead should only be noticeable in, e.g. fortified calls inside a
hot loop. In theory expressions to compute sizes could be arbitrarily
complex, but I haven't seen any cases yet that were large enough to be a
bother. I reckon if we find such use cases, it'll be in the kernel but
even then it may not be worth downgrading fortification across all
sources just for those specific hot paths. So I agree, the user
override isn't critically necessary.
FWIW,
Reviewed-by: Siddhesh Poyarekar <[email protected]>
Thanks,
Sid
On 2022-09-20 15:22, 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 <[email protected]>
> Cc: Siddhesh Poyarekar <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Nick Desaulniers <[email protected]>
> Cc: Nathan Chancellor <[email protected]>
> Cc: Tom Rix <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
> ---
> drivers/misc/lkdtm/heap.c | 1 +
> include/linux/compiler_attributes.h | 5 +++++
> include/linux/fortify-string.h | 7 +++++++
> 3 files changed, 13 insertions(+)
Hi Kees,
Circling back on this, I noticed that all but this patch got pulled into
Linus' tree. Is there a reason why this has been held back?
Thanks,
Sid
On Tue, Nov 22, 2022 at 05:20:37AM -0500, Siddhesh Poyarekar wrote:
> On 2022-09-20 15:22, 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 <[email protected]>
> > Cc: Siddhesh Poyarekar <[email protected]>
> > Cc: Arnd Bergmann <[email protected]>
> > Cc: Nick Desaulniers <[email protected]>
> > Cc: Nathan Chancellor <[email protected]>
> > Cc: Tom Rix <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Signed-off-by: Kees Cook <[email protected]>
> > ---
> > drivers/misc/lkdtm/heap.c | 1 +
> > include/linux/compiler_attributes.h | 5 +++++
> > include/linux/fortify-string.h | 7 +++++++
> > 3 files changed, 13 insertions(+)
>
> Hi Kees,
>
> Circling back on this, I noticed that all but this patch got pulled into
> Linus' tree. Is there a reason why this has been held back?
Hi!
Yeah, it depended on a bunch of various clean-ups, which have finally
managed to land. It's late enough in the devel cycle that I suspect I
will hold this one back until after the merge window and then make sure
it has plenty of time to bake in -next. If the rest of the patches
continue to behave, I may change my mind... :)
-Kees
--
Kees Cook
On 2022-11-23 00:15, Kees Cook wrote:
> Yeah, it depended on a bunch of various clean-ups, which have finally
> managed to land. It's late enough in the devel cycle that I suspect I
> will hold this one back until after the merge window and then make sure
> it has plenty of time to bake in -next. If the rest of the patches
> continue to behave, I may change my mind... :)
OK, thanks for responding!
Sid
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 <[email protected]>
> Cc: Siddhesh Poyarekar <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Nick Desaulniers <[email protected]>
> Cc: Nathan Chancellor <[email protected]>
> Cc: Tom Rix <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
> ---
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
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 <[email protected]>
> > Cc: Siddhesh Poyarekar <[email protected]>
> > Cc: Arnd Bergmann <[email protected]>
> > Cc: Nick Desaulniers <[email protected]>
> > Cc: Nathan Chancellor <[email protected]>
> > Cc: Tom Rix <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Signed-off-by: Kees Cook <[email protected]>
> > ---
>
> 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
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
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 <[email protected]>
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