Skip to contentSkip to conversation

splat in ikheaders_read (bpftrace)

DISCUSSION Jakub Kicinski 3 participants 7 messages Mar 2, 2023 BPF

lore
Share & tools
lore

Jakub Kicinski

author
#0 lore raw

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
#1 lore raw
Quoted 4 lines · 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:

Can you give me an example command line to try to repro this?

Quoted 2 lines
[ 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).

Quoted 5 lines
[ 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

Signature · 1 line
Kees Cook
author
#2 lore raw
Quoted 7 lines · On Thu, 02 Mar 2023 13:57:57 -0800 Kees Cook wrote:
Quoted 4 lines · 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:

Can you give me an example command line to try to repro this?

cat /sys/kernel/kheaders.tar.xz >> /dev/null

Quoted 3 lines
Quoted 1 line
[ 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.

Quoted 10 lines
Quoted 5 lines
[ 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)

Quoted 6 lines
{
	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;

}

author
#3 lore raw
Quoted 15 lines · On Thu, 2 Mar 2023 14:08:14 -0800 Jakub Kicinski wrote:
Quoted 10 lines

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:

Review ikheaders_read(struct file *file, struct kobject *kobj, · line 41

Quoted 7 lines
(quoted diff) +1 -0
@@ -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?

author
#4 lore raw
Quoted 4 lines · On Thu, 2 Mar 2023 14:12:31 -0800 Jakub Kicinski wrote:
Quoted 3 lines · 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:

Quoted 12 lines

Review ikheaders_read(struct file *file, struct kobject *kobj, · line 41

Quoted 8 lines
(quoted diff) +1 -0
@@ -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 <[email protected]>
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 <[email protected]>
---
 kernel/kheaders.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
kernel/kheaders.c +3 -3
index 8f69772af77b..90562eb4093a 100644
@@ -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);
 }
Signature · 1 line
2.39.2
#5 lore raw
In reply to Jakub Kicinski
Quoted 15 lines · Jakub Kicinski
Quoted 14 lines · On Thu, 2 Mar 2023 14:08:14 -0800 Jakub Kicinski wrote:
Quoted 10 lines

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:

Quoted 12 lines

Review ikheaders_read(struct file *file, struct kobject *kobj, · line 41

Quoted 8 lines
(quoted diff) +1 -0
@@ -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.)

kernel/kheaders.c +5 -5
index 8f69772af77b..42163c9e94e5 100644
@@ -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);
 }
Signature · 1 line
Kees Cook
author
#6 lore raw
Quoted 7 lines · 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!