* splat in ikheaders_read (bpftrace) @ 2023-03-02 19:21 Jakub Kicinski 2023-03-02 21:57 ` Kees Cook 0 siblings, 1 reply; 7+ messages in thread From: Jakub Kicinski @ 2023-03-02 19:21 UTC (permalink / raw) To: Kees Cook; +Cc: linux-hardening, bpf Hi Kees! Running tests on net (Linus's tree as of Monday) I get this splat trying to attach bpftrace to a tracepoint: [ 2468.945793] kernel BUG at lib/string_helpers.c:1027! [ 2468.946602] invalid opcode: 0000 [#8] SMP KASAN [ 2468.947416] CPU: 1 PID: 1094 Comm: tar Tainted: G D 6.2.0-12879-g040b4d2ce1ad #646 [ 2468.948547] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.1-2.fc37 04/01/2014 [ 2468.949683] RIP: 0010:fortify_panic+0xf/0x20 [ 2468.950291] Code: 85 ff 75 d3 bb ea ff ff ff 89 d8 5b 5d 41 5c 41 5d 41 5e c3 0f 1f 80 00 00 00 00 48 89 fe 48 c7 c7 c0 dd 6b a6 e8 01 73 90 ff <0f> 0b 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 41 54 55 53 48 [ 2468.952438] RSP: 0018:ffff888011c77d10 EFLAGS: 00010246 [ 2468.953125] RAX: 0000000000000022 RBX: ffff8880129fd400 RCX: ffffffffa528008e [ 2468.954022] RDX: 0000000000000000 RSI: 0000000000000008 RDI: ffff88806d33338c [ 2468.954935] RBP: ffff888011c77e00 R08: 0000000000000001 R09: ffff888011c77b67 [ 2468.955788] R10: ffffed100238ef6c R11: 7970636d656d6564 R12: ffff888011c77db0 [ 2468.956719] R13: ffff888011d5e000 R14: ffffffffa5716ef0 R15: ffff8880129fd558 [ 2468.957678] FS: 00007fd375e3d280(0000) GS:ffff88806d300000(0000) knlGS:0000000000000000 [ 2468.958729] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 2468.959482] CR2: 000055b2b8ad81c8 CR3: 000000000f07e006 CR4: 0000000000370ee0 [ 2468.960318] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 2468.961109] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [ 2468.961930] Call Trace: [ 2468.962300] <TASK> [ 2468.962611] ikheaders_read+0x45/0x50 [kheaders] [ 2468.963178] kernfs_fop_read_iter+0x1a4/0x2f0 [ 2468.963724] vfs_read+0x39f/0x4b0 [ 2468.964127] ? kernel_read+0xc0/0xc0 [ 2468.964563] ? build_open_flags+0x230/0x230 [ 2468.965041] ? __fget_light+0xd7/0xf0 [ 2468.965521] ksys_read+0xc7/0x160 [ 2468.965905] ? __ia32_sys_pwrite64+0x140/0x140 [ 2468.966385] do_syscall_64+0x34/0x80 [ 2468.966834] entry_SYSCALL_64_after_hwframe+0x46/0xb0 [ 2468.967490] RIP: 0033:0x7fd375d01852 [ 2468.968903] Code: c0 e9 b2 fe ff ff 50 48 8d 3d 9a d0 0b 00 e8 55 f6 01 00 0f 1f 44 00 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 0f 05 <48> 3d 00 f0 ff ff 77 56 c3 0f 1f 44 00 00 48 83 ec 28 48 89 54 24 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: splat in ikheaders_read (bpftrace) 2023-03-02 19:21 splat in ikheaders_read (bpftrace) Jakub Kicinski @ 2023-03-02 21:57 ` Kees Cook 2023-03-02 22:08 ` Jakub Kicinski 0 siblings, 1 reply; 7+ messages in thread From: Kees Cook @ 2023-03-02 21:57 UTC (permalink / raw) To: Jakub Kicinski, Kees Cook; +Cc: linux-hardening, bpf On March 2, 2023 11:21:30 AM PST, Jakub Kicinski <kuba@kernel.org> wrote: >Hi Kees! > >Running tests on net (Linus's tree as of Monday) I get this splat >trying to attach bpftrace to a tracepoint: Can you give me an example command line to try to repro this? > >[ 2468.945793] kernel BUG at lib/string_helpers.c:1027! Were there any lines above this? It must be memcpy blowing up due to what it thinks is an overflow from having tracked an allocation size (that's the major change this cycle). >[ 2468.949683] RIP: 0010:fortify_panic+0xf/0x20 >... >[ 2468.961930] Call Trace: >[ 2468.962300] <TASK> >[ 2468.962611] ikheaders_read+0x45/0x50 [kheaders] static ssize_t ikheaders_read(struct file *file, struct kobject *kobj, struct bin_attribute *bin_attr, char *buf, loff_t off, size_t len) { memcpy(buf, &kernel_headers_data + off, len); return len; } I will take a look at the caller's allocation of "buf" and kernel_headers_data. -Kees -- Kees Cook ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: splat in ikheaders_read (bpftrace) 2023-03-02 21:57 ` Kees Cook @ 2023-03-02 22:08 ` Jakub Kicinski 2023-03-02 22:12 ` Jakub Kicinski 0 siblings, 1 reply; 7+ messages in thread From: Jakub Kicinski @ 2023-03-02 22:08 UTC (permalink / raw) To: Kees Cook; +Cc: Kees Cook, linux-hardening, bpf On Thu, 02 Mar 2023 13:57:57 -0800 Kees Cook wrote: > On March 2, 2023 11:21:30 AM PST, Jakub Kicinski <kuba@kernel.org> wrote: > >Hi Kees! > > > >Running tests on net (Linus's tree as of Monday) I get this splat > >trying to attach bpftrace to a tracepoint: > > Can you give me an example command line to try to repro this? cat /sys/kernel/kheaders.tar.xz >> /dev/null > >[ 2468.945793] kernel BUG at lib/string_helpers.c:1027! > > Were there any lines above this? It must be memcpy blowing up due to what it thinks is an overflow from having tracked an allocation size (that's the major change this cycle). Yes detected buffer overflow in memcpy sorry. > >[ 2468.949683] RIP: 0010:fortify_panic+0xf/0x20 > >... > >[ 2468.961930] Call Trace: > >[ 2468.962300] <TASK> > >[ 2468.962611] ikheaders_read+0x45/0x50 [kheaders] > > static ssize_t > ikheaders_read(struct file *file, struct kobject *kobj, > struct bin_attribute *bin_attr, > char *buf, loff_t off, size_t len) > { > memcpy(buf, &kernel_headers_data + off, len); > return len; > } > > I will take a look at the caller's allocation of "buf" and kernel_headers_data. Mm. Actually stopping to look at the code - I don't see it bound checking against kernel_headers_data_end :| Maybe we need: @@ -34,6 +35,7 @@ ikheaders_read(struct file *file, struct kobject *kobj, struct bin_attribute *bin_attr, char *buf, loff_t off, size_t len) { + len = min_t(size_t, kernel_headers_data_end - kernel_headers_data, len); memcpy(buf, &kernel_headers_data + off, len); return len; } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: splat in ikheaders_read (bpftrace) 2023-03-02 22:08 ` Jakub Kicinski @ 2023-03-02 22:12 ` Jakub Kicinski 2023-03-02 22:35 ` Jakub Kicinski 2023-03-02 22:39 ` Kees Cook 0 siblings, 2 replies; 7+ messages in thread From: Jakub Kicinski @ 2023-03-02 22:12 UTC (permalink / raw) To: Kees Cook; +Cc: Kees Cook, linux-hardening, bpf On Thu, 2 Mar 2023 14:08:14 -0800 Jakub Kicinski wrote: > > static ssize_t > > ikheaders_read(struct file *file, struct kobject *kobj, > > struct bin_attribute *bin_attr, > > char *buf, loff_t off, size_t len) > > { > > memcpy(buf, &kernel_headers_data + off, len); > > return len; > > } > > > > I will take a look at the caller's allocation of "buf" and kernel_headers_data. > > Mm. Actually stopping to look at the code - I don't see it bound > checking against kernel_headers_data_end :| Maybe we need: > > @@ -34,6 +35,7 @@ ikheaders_read(struct file *file, struct kobject *kobj, > struct bin_attribute *bin_attr, > char *buf, loff_t off, size_t len) > { > + len = min_t(size_t, kernel_headers_data_end - kernel_headers_data, len); > memcpy(buf, &kernel_headers_data + off, len); > return len; > } Scratch that, the size is set at init time. My guess was memcpy() thinks the size of kernel_headers_data is 1 since it's declared as char? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: splat in ikheaders_read (bpftrace) 2023-03-02 22:12 ` Jakub Kicinski @ 2023-03-02 22:35 ` Jakub Kicinski 2023-03-02 22:39 ` Kees Cook 1 sibling, 0 replies; 7+ messages in thread From: Jakub Kicinski @ 2023-03-02 22:35 UTC (permalink / raw) To: Kees Cook; +Cc: Kees Cook, linux-hardening, bpf, Joel Fernandes (Google) On Thu, 2 Mar 2023 14:12:31 -0800 Jakub Kicinski wrote: > On Thu, 2 Mar 2023 14:08:14 -0800 Jakub Kicinski wrote: > > Mm. Actually stopping to look at the code - I don't see it bound > > checking against kernel_headers_data_end :| Maybe we need: > > > > @@ -34,6 +35,7 @@ ikheaders_read(struct file *file, struct kobject *kobj, > > struct bin_attribute *bin_attr, > > char *buf, loff_t off, size_t len) > > { > > + len = min_t(size_t, kernel_headers_data_end - kernel_headers_data, len); > > memcpy(buf, &kernel_headers_data + off, len); > > return len; > > } > > Scratch that, the size is set at init time. > My guess was memcpy() thinks the size of kernel_headers_data > is 1 since it's declared as char? Like this? ---->8-------------------- From 7549153017144281c2475d9370962ad4d0ea8d4c Mon Sep 17 00:00:00 2001 From: Jakub Kicinski <kuba@kernel.org> Date: Thu, 2 Mar 2023 11:15:53 -0800 Subject: [PATCH] kheaders: use a flex array for the symbol Kernels with hardened copy hit: detected buffer overflow in memcpy kernel BUG at lib/string_helpers.c:1027! invalid opcode: 0000 [#8] SMP KASAN CPU: 1 PID: 1094 Comm: tar Tainted: G D 6.2.0-12879-g040b4d2ce1ad #646 RIP: 0010:fortify_panic+0xf/0x20 [...] Call Trace: <TASK> ikheaders_read+0x45/0x50 [kheaders] kernfs_fop_read_iter+0x1a4/0x2f0 Repro: $ cat /sys/kernel/kheaders.tar.xz >> /dev/null Fixes: 43d8ce9d65a5 ("Provide in-kernel headers to make extending kernel easier") Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- kernel/kheaders.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/kernel/kheaders.c b/kernel/kheaders.c index 8f69772af77b..90562eb4093a 100644 --- a/kernel/kheaders.c +++ b/kernel/kheaders.c @@ -26,7 +26,7 @@ asm ( " .popsection \n" ); -extern char kernel_headers_data; +extern char kernel_headers_data[]; extern char kernel_headers_data_end; static ssize_t @@ -34,7 +34,7 @@ ikheaders_read(struct file *file, struct kobject *kobj, struct bin_attribute *bin_attr, char *buf, loff_t off, size_t len) { - memcpy(buf, &kernel_headers_data + off, len); + memcpy(buf, &kernel_headers_data[off], len); return len; } @@ -49,7 +49,7 @@ static struct bin_attribute kheaders_attr __ro_after_init = { static int __init ikheaders_init(void) { kheaders_attr.size = (&kernel_headers_data_end - - &kernel_headers_data); + &kernel_headers_data[0]); return sysfs_create_bin_file(kernel_kobj, &kheaders_attr); } -- 2.39.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: splat in ikheaders_read (bpftrace) 2023-03-02 22:12 ` Jakub Kicinski 2023-03-02 22:35 ` Jakub Kicinski @ 2023-03-02 22:39 ` Kees Cook 2023-03-02 22:41 ` Jakub Kicinski 1 sibling, 1 reply; 7+ messages in thread From: Kees Cook @ 2023-03-02 22:39 UTC (permalink / raw) To: Jakub Kicinski; +Cc: Kees Cook, linux-hardening, bpf On Thu, Mar 02, 2023 at 02:12:31PM -0800, Jakub Kicinski wrote: > On Thu, 2 Mar 2023 14:08:14 -0800 Jakub Kicinski wrote: > > > static ssize_t > > > ikheaders_read(struct file *file, struct kobject *kobj, > > > struct bin_attribute *bin_attr, > > > char *buf, loff_t off, size_t len) > > > { > > > memcpy(buf, &kernel_headers_data + off, len); > > > return len; > > > } > > > > > > I will take a look at the caller's allocation of "buf" and kernel_headers_data. > > > > Mm. Actually stopping to look at the code - I don't see it bound > > checking against kernel_headers_data_end :| Maybe we need: > > > > @@ -34,6 +35,7 @@ ikheaders_read(struct file *file, struct kobject *kobj, > > struct bin_attribute *bin_attr, > > char *buf, loff_t off, size_t len) > > { > > + len = min_t(size_t, kernel_headers_data_end - kernel_headers_data, len); > > memcpy(buf, &kernel_headers_data + off, len); > > return len; > > } > > Scratch that, the size is set at init time. > My guess was memcpy() thinks the size of kernel_headers_data > is 1 since it's declared as char? I've improved the reporting, and yeah, it's due to the declaration: memcpy: detected buffer overflow: 4096 byte read from buffer of size 1 But that's an easy fix -- this has been done in lots of other places. It needs to be an array, not a single char. (I'm surprised we hadn't seen this before.) diff --git a/kernel/kheaders.c b/kernel/kheaders.c index 8f69772af77b..42163c9e94e5 100644 --- a/kernel/kheaders.c +++ b/kernel/kheaders.c @@ -26,15 +26,15 @@ asm ( " .popsection \n" ); -extern char kernel_headers_data; -extern char kernel_headers_data_end; +extern char kernel_headers_data[]; +extern char kernel_headers_data_end[]; static ssize_t ikheaders_read(struct file *file, struct kobject *kobj, struct bin_attribute *bin_attr, char *buf, loff_t off, size_t len) { - memcpy(buf, &kernel_headers_data + off, len); + memcpy(buf, &kernel_headers_data[off], len); return len; } @@ -48,8 +48,8 @@ static struct bin_attribute kheaders_attr __ro_after_init = { static int __init ikheaders_init(void) { - kheaders_attr.size = (&kernel_headers_data_end - - &kernel_headers_data); + kheaders_attr.size = (kernel_headers_data_end - + kernel_headers_data); return sysfs_create_bin_file(kernel_kobj, &kheaders_attr); } -- Kees Cook ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: splat in ikheaders_read (bpftrace) 2023-03-02 22:39 ` Kees Cook @ 2023-03-02 22:41 ` Jakub Kicinski 0 siblings, 0 replies; 7+ messages in thread From: Jakub Kicinski @ 2023-03-02 22:41 UTC (permalink / raw) To: Kees Cook; +Cc: Kees Cook, linux-hardening, bpf On Thu, 2 Mar 2023 14:39:17 -0800 Kees Cook wrote: > I've improved the reporting, and yeah, it's due to the declaration: > > memcpy: detected buffer overflow: 4096 byte read from buffer of size 1 > > But that's an easy fix -- this has been done in lots of other places. It > needs to be an array, not a single char. (I'm surprised we hadn't seen > this before.) LGTM, feel free to add my Tested-by, if that matters :) Thanks! ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-03-02 22:41 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-03-02 19:21 splat in ikheaders_read (bpftrace) Jakub Kicinski 2023-03-02 21:57 ` Kees Cook 2023-03-02 22:08 ` Jakub Kicinski 2023-03-02 22:12 ` Jakub Kicinski 2023-03-02 22:35 ` Jakub Kicinski 2023-03-02 22:39 ` Kees Cook 2023-03-02 22:41 ` Jakub Kicinski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox