splat in ikheaders_read (bpftrace)

splat in ikheaders_read (bpftrace)

[Jakub Kicinski]

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

Re: splat in ikheaders_read (bpftrace)

[Kees Cook]

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

Re: splat in ikheaders_read (bpftrace)

[Jakub Kicinski]

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;
 }

Re: splat in ikheaders_read (bpftrace)

[Jakub Kicinski]

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?

Re: splat in ikheaders_read (bpftrace)

[Jakub Kicinski]

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

Re: splat in ikheaders_read (bpftrace)

[Kees Cook]

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

Re: splat in ikheaders_read (bpftrace)

[Jakub Kicinski]

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!