[PATCH bpf] bpf: add check for invalid name in btf_name_valid_section()

[PATCH bpf] bpf: add check for invalid name in btf_name_valid_section()

[Jeongjun Park]

If the length of the name string is 1 and the value of name[0] is NULL 
byte, an OOB vulnerability occurs in btf_name_valid_section() and the 
return value is true, so the invalid name passes the check. 

To solve this, you need to check if the first position is NULL byte.

Fixes: bd70a8fb7ca4 ("bpf: Allow all printable characters in BTF DATASEC names")
Signed-off-by: Jeongjun Park <aha310510@gmail.com>
---
 kernel/bpf/btf.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 520f49f422fe..5c24ea1a65a4 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -823,6 +823,9 @@ static bool btf_name_valid_section(const struct btf *btf, u32 offset)
 	const char *src = btf_str_by_offset(btf, offset);
 	const char *src_limit;
 
+	if (!*src)
+		return false;
+
 	/* set a limit on identifier length */
 	src_limit = src + KSYM_NAME_LEN;
 	src++;
--

Re: [PATCH bpf] bpf: add check for invalid name in btf_name_valid_section()

[Alexei Starovoitov]

On Fri, Aug 23, 2024 at 3:43 AM Jeongjun Park <aha310510@gmail.com> wrote:
>
> If the length of the name string is 1 and the value of name[0] is NULL
> byte, an OOB vulnerability occurs in btf_name_valid_section() and the
> return value is true, so the invalid name passes the check.
>
> To solve this, you need to check if the first position is NULL byte.
>
> Fixes: bd70a8fb7ca4 ("bpf: Allow all printable characters in BTF DATASEC names")
> Signed-off-by: Jeongjun Park <aha310510@gmail.com>
> ---
>  kernel/bpf/btf.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 520f49f422fe..5c24ea1a65a4 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -823,6 +823,9 @@ static bool btf_name_valid_section(const struct btf *btf, u32 offset)
>         const char *src = btf_str_by_offset(btf, offset);
>         const char *src_limit;
>
> +       if (!*src)
> +               return false;
> +

We've talked about it. Quote:
"Pls add a selftest that demonstrates the issue
and produce a patch to fix just that."

length == 1 and name[0] = 0 is a hypothesis.
Demonstrate that such a scenario is possible then this patch will be
worth applying.

pw-bot: cr

Re: [PATCH bpf] bpf: add check for invalid name in btf_name_valid_section()

[Jeongjun Park]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 11537 bytes --]

Alexei Starovoitov wrote:
>
> On Fri, Aug 23, 2024 at 3:43 AM Jeongjun Park <aha310510@gmail.com> wrote:
> >
> > If the length of the name string is 1 and the value of name[0] is NULL
> > byte, an OOB vulnerability occurs in btf_name_valid_section() and the
> > return value is true, so the invalid name passes the check.
> >
> > To solve this, you need to check if the first position is NULL byte.
> >
> > Fixes: bd70a8fb7ca4 ("bpf: Allow all printable characters in BTF DATASEC names")
> > Signed-off-by: Jeongjun Park <aha310510@gmail.com>
> > ---
> >  kernel/bpf/btf.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 520f49f422fe..5c24ea1a65a4 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -823,6 +823,9 @@ static bool btf_name_valid_section(const struct btf *btf, u32 offset)
> >         const char *src = btf_str_by_offset(btf, offset);
> >         const char *src_limit;
> >
> > +       if (!*src)
> > +               return false;
> > +
>
> We've talked about it. Quote:
> "Pls add a selftest that demonstrates the issue
> and produce a patch to fix just that."
>
> length == 1 and name[0] = 0 is a hypothesis.
> Demonstrate that such a scenario is possible then this patch will be
> worth applying.
>
> pw-bot: cr

Sorry for the omission, I still don't know how to write selftest.

But I can give you the C repro and KASAN log that trigger this vulnerability. 
I would appreciate it if you could look at it and make a judgment.

Regards,
Jeongjun Park

C repro:
// gcc -o repro repro.c
#define _GNU_SOURCE 

#include <endian.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/syscall.h>
#include <sys/types.h>
#include <unistd.h>

#ifndef __NR_bpf
#define __NR_bpf 321
#endif

int main(void)
{
		syscall(__NR_mmap, /*addr=*/0x1ffff000ul, /*len=*/0x1000ul, /*prot=*/0ul, /*flags=MAP_FIXED|MAP_ANONYMOUS|MAP_PRIVATE*/0x32ul, /*fd=*/-1, /*offset=*/0ul);
	syscall(__NR_mmap, /*addr=*/0x20000000ul, /*len=*/0x1000000ul, /*prot=PROT_WRITE|PROT_READ|PROT_EXEC*/7ul, /*flags=MAP_FIXED|MAP_ANONYMOUS|MAP_PRIVATE*/0x32ul, /*fd=*/-1, /*offset=*/0ul);
	syscall(__NR_mmap, /*addr=*/0x21000000ul, /*len=*/0x1000ul, /*prot=*/0ul, /*flags=MAP_FIXED|MAP_ANONYMOUS|MAP_PRIVATE*/0x32ul, /*fd=*/-1, /*offset=*/0ul);
	const char* reason;
	(void)reason;
				if (write(1, "executing program\n", sizeof("executing program\n") - 1)) {}
	*(uint64_t*)0x20000340 = 0x20000140;
	*(uint16_t*)0x20000140 = 0xeb9f;
	*(uint8_t*)0x20000142 = 1;
	*(uint8_t*)0x20000143 = 0;
	*(uint32_t*)0x20000144 = 0x18;
	*(uint32_t*)0x20000148 = 0;
	*(uint32_t*)0x2000014c = 0xc4;
	*(uint32_t*)0x20000150 = 0xc4;
	*(uint32_t*)0x20000154 = 5;
	*(uint32_t*)0x20000158 = 4;
	*(uint16_t*)0x2000015c = 0xa;
	*(uint8_t*)0x2000015e = 0;
	*(uint8_t*)0x2000015f = 0xf;
	*(uint32_t*)0x20000160 = 1;
	*(uint32_t*)0x20000164 = 3;
	*(uint32_t*)0x20000168 = 1;
	*(uint32_t*)0x2000016c = 0x9247;
	*(uint32_t*)0x20000170 = 3;
	*(uint32_t*)0x20000174 = 0xfffffff9;
	*(uint32_t*)0x20000178 = 9;
	*(uint32_t*)0x2000017c = 5;
	*(uint32_t*)0x20000180 = 0x7d;
	*(uint32_t*)0x20000184 = 0x48;
	*(uint32_t*)0x20000188 = 3;
	*(uint32_t*)0x2000018c = 9;
	*(uint32_t*)0x20000190 = 2;
	*(uint32_t*)0x20000194 = 1;
	*(uint32_t*)0x20000198 = 0xe;
	*(uint32_t*)0x2000019c = 5;
	*(uint32_t*)0x200001a0 = 3;
	*(uint32_t*)0x200001a4 = 0xfffffa64;
	*(uint32_t*)0x200001a8 = 0xffff;
	*(uint32_t*)0x200001ac = 3;
	*(uint32_t*)0x200001b0 = 3;
	*(uint32_t*)0x200001b4 = 0xfff;
	*(uint32_t*)0x200001b8 = 5;
	*(uint32_t*)0x200001bc = 0x8fd;
	*(uint32_t*)0x200001c0 = 0xa16;
	*(uint32_t*)0x200001c4 = 2;
	*(uint32_t*)0x200001c8 = 7;
	*(uint32_t*)0x200001cc = 6;
	*(uint32_t*)0x200001d0 = 2;
	*(uint32_t*)0x200001d4 = 0xe821;
	*(uint32_t*)0x200001d8 = 5;
	memset((void*)0x200001dc, 104, 1);
	*(uint32_t*)0x200001dd = 0xf;
	*(uint16_t*)0x200001e1 = 0;
	*(uint8_t*)0x200001e3 = 0;
	*(uint8_t*)0x200001e4 = 8;
	*(uint32_t*)0x200001e5 = 4;
	*(uint32_t*)0x200001e9 = 0;
	*(uint16_t*)0x200001ed = 0;
	*(uint8_t*)0x200001ef = 0;
	*(uint8_t*)0x200001f0 = 3;
	*(uint32_t*)0x200001f1 = 0;
	*(uint32_t*)0x200001f5 = 4;
	*(uint32_t*)0x200001f9 = 3;
	*(uint32_t*)0x200001fd = 2;
	*(uint32_t*)0x20000201 = 7;
	*(uint16_t*)0x20000205 = 0;
	*(uint8_t*)0x20000207 = 0;
	*(uint8_t*)0x20000208 = 0xa;
	*(uint32_t*)0x20000209 = 5;
	*(uint32_t*)0x2000020d = 3;
	*(uint16_t*)0x20000211 = 0;
	*(uint8_t*)0x20000213 = 0;
	*(uint8_t*)0x20000214 = 0xf;
	*(uint32_t*)0x20000215 = 3;
	memcpy((void*)0x20000219, "\x65\xd8\x38", 3);
	*(uint8_t*)0x2000021c = 0;
	*(uint8_t*)0x2000021d = 0;
	*(uint8_t*)0x2000021e = 0;
	*(uint8_t*)0x2000021f = 0;
	*(uint8_t*)0x20000220 = 0;
	*(uint64_t*)0x20000348 = 0;
	*(uint32_t*)0x20000350 = 0xe1;
	*(uint32_t*)0x20000354 = 0;
	*(uint32_t*)0x20000358 = 0;
	*(uint32_t*)0x2000035c = 0;
	syscall(__NR_bpf, /*cmd=*/0x12ul, /*arg=*/0x20000340ul, /*size=*/0x20ul);
	return 0;
}

KASAN log:

==================================================================
BUG: KASAN: slab-out-of-bounds in btf_name_valid_section kernel/bpf/btf.c:829 [inline]
BUG: KASAN: slab-out-of-bounds in btf_datasec_check_meta+0x7f8/0x880 kernel/bpf/btf.c:4698
Read of size 1 at addr ffff888012faaeb8 by task syz.3.2008/25618

CPU: 0 UID: 0 PID: 25618 Comm: syz.3.2008 Not tainted 6.11.0-rc5 #12
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:93 [inline]
 dump_stack_lvl+0x116/0x1b0 lib/dump_stack.c:119
 print_address_description mm/kasan/report.c:377 [inline]
 print_report+0xc0/0x5e0 mm/kasan/report.c:488
 kasan_report+0xbd/0xf0 mm/kasan/report.c:601
 btf_name_valid_section kernel/bpf/btf.c:829 [inline]
 btf_datasec_check_meta+0x7f8/0x880 kernel/bpf/btf.c:4698
 btf_check_meta kernel/bpf/btf.c:5180 [inline]
 btf_check_all_metas kernel/bpf/btf.c:5204 [inline]
 btf_parse_type_sec kernel/bpf/btf.c:5340 [inline]
 btf_parse kernel/bpf/btf.c:5732 [inline]
 btf_new_fd+0x1764/0x4c30 kernel/bpf/btf.c:7650
 bpf_btf_load kernel/bpf/syscall.c:5035 [inline]
 __sys_bpf+0x1dc4/0x5040 kernel/bpf/syscall.c:5755
 __do_sys_bpf kernel/bpf/syscall.c:5817 [inline]
 __se_sys_bpf kernel/bpf/syscall.c:5815 [inline]
 __x64_sys_bpf+0x78/0xc0 kernel/bpf/syscall.c:5815
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xcb/0x250 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7f6dee39712d
Code: 02 b8 ff ff ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 a8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f6def181fa8 EFLAGS: 00000246 ORIG_RAX: 0000000000000141
RAX: ffffffffffffffda RBX: 00007f6dee535f80 RCX: 00007f6dee39712d
RDX: 0000000000000020 RSI: 0000000020000600 RDI: 0000000000000012
RBP: 00007f6dee41bd8a R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 000000000000000b R14: 00007f6dee535f80 R15: 00007f6def162000
 </TASK>

Allocated by task 25618:
 kasan_save_stack+0x24/0x50 mm/kasan/common.c:47
 kasan_save_track+0x14/0x30 mm/kasan/common.c:68
 poison_kmalloc_redzone mm/kasan/common.c:370 [inline]
 __kasan_kmalloc+0xaa/0xb0 mm/kasan/common.c:387
 kasan_kmalloc include/linux/kasan.h:211 [inline]
 __do_kmalloc_node mm/slub.c:4158 [inline]
 __kmalloc_node_noprof+0x20a/0x450 mm/slub.c:4164
 __kvmalloc_node_noprof+0x148/0x1b0 mm/util.c:650
 btf_parse kernel/bpf/btf.c:5708 [inline]
 btf_new_fd+0x5f9/0x4c30 kernel/bpf/btf.c:7650
 bpf_btf_load kernel/bpf/syscall.c:5035 [inline]
 __sys_bpf+0x1dc4/0x5040 kernel/bpf/syscall.c:5755
 __do_sys_bpf kernel/bpf/syscall.c:5817 [inline]
 __se_sys_bpf kernel/bpf/syscall.c:5815 [inline]
 __x64_sys_bpf+0x78/0xc0 kernel/bpf/syscall.c:5815
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xcb/0x250 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

The buggy address belongs to the object at ffff888012faae00
 which belongs to the cache kmalloc-192 of size 192
The buggy address is located 0 bytes to the right of
 allocated 184-byte region [ffff888012faae00, ffff888012faaeb8)

The buggy address belongs to the physical page:
page: refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x12faa
flags: 0xfff00000000000(node=0|zone=1|lastcpupid=0x7ff)
page_type: 0xfdffffff(slab)
raw: 00fff00000000000 ffff8880154413c0 ffffea00004b78c0 dead000000000002
raw: 0000000000000000 0000000080100010 00000001fdffffff 0000000000000000
page dumped because: kasan: bad access detected
page_owner tracks the page as allocated
page last allocated via order 0, migratetype Unmovable, gfp_mask 0x52cc0(GFP_KERNEL|__GFP_NOWARN|__GFP_NORETRY|__GFP_COMP), pid 8387, tgid 8387 (syz-executor), ts 57078807121, free_ts 54812229064
 set_page_owner include/linux/page_owner.h:32 [inline]
 post_alloc_hook+0x2e7/0x350 mm/page_alloc.c:1493
 prep_new_page mm/page_alloc.c:1501 [inline]
 get_page_from_freelist+0xbf3/0x2850 mm/page_alloc.c:3439
 __alloc_pages_noprof+0x214/0x21e0 mm/page_alloc.c:4695
 __alloc_pages_node_noprof include/linux/gfp.h:269 [inline]
 alloc_pages_node_noprof include/linux/gfp.h:296 [inline]
 alloc_slab_page+0x55/0x100 mm/slub.c:2321
 allocate_slab mm/slub.c:2484 [inline]
 new_slab+0x83/0x260 mm/slub.c:2537
 ___slab_alloc+0xbb7/0x1850 mm/slub.c:3723
 __slab_alloc.constprop.0+0x56/0xb0 mm/slub.c:3813
 __slab_alloc_node mm/slub.c:3866 [inline]
 slab_alloc_node mm/slub.c:4025 [inline]
 __kmalloc_cache_noprof+0x2be/0x310 mm/slub.c:4184
 kmalloc_noprof include/linux/slab.h:681 [inline]
 kzalloc_noprof include/linux/slab.h:807 [inline]
 kset_create lib/kobject.c:965 [inline]
 kset_create_and_add+0x4f/0x1a0 lib/kobject.c:1008
 register_queue_kobjects net/core/net-sysfs.c:1887 [inline]
 netdev_register_kobject+0x1d2/0x400 net/core/net-sysfs.c:2140
 register_netdevice+0x1329/0x1bf0 net/core/dev.c:10444
 veth_newlink+0x4bd/0x960 drivers/net/veth.c:1860
 rtnl_newlink_create net/core/rtnetlink.c:3510 [inline]
 __rtnl_newlink+0x1138/0x18e0 net/core/rtnetlink.c:3730
 rtnl_newlink+0x68/0xa0 net/core/rtnetlink.c:3743
 rtnetlink_rcv_msg+0x3c9/0xfb0 net/core/rtnetlink.c:6647
 netlink_rcv_skb+0x165/0x440 net/netlink/af_netlink.c:2550
page last free pid 96 tgid 96 stack trace:
 reset_page_owner include/linux/page_owner.h:25 [inline]
 free_pages_prepare mm/page_alloc.c:1094 [inline]
 free_unref_folios+0x9c6/0x1320 mm/page_alloc.c:2660
 shrink_folio_list+0x29f0/0x4260 mm/vmscan.c:1530
 evict_folios+0x71e/0x1b50 mm/vmscan.c:4580
 try_to_shrink_lruvec+0x62b/0x9a0 mm/vmscan.c:4775
 shrink_one+0x417/0x7c0 mm/vmscan.c:4813
 shrink_many mm/vmscan.c:4876 [inline]
 lru_gen_shrink_node mm/vmscan.c:4954 [inline]
 shrink_node+0x244d/0x3830 mm/vmscan.c:5934
 kswapd_shrink_node mm/vmscan.c:6762 [inline]
 balance_pgdat+0xba2/0x1880 mm/vmscan.c:6954
 kswapd+0x702/0xd50 mm/vmscan.c:7223
 kthread+0x2c7/0x3b0 kernel/kthread.c:389
 ret_from_fork+0x45/0x80 arch/x86/kernel/process.c:147
 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244

Memory state around the buggy address:
 ffff888012faad80: 00 fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff888012faae00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>ffff888012faae80: 00 00 00 00 00 00 00 fc fc fc fc fc fc fc fc fc
                                        ^
 ffff888012faaf00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 ffff888012faaf80: 00 00 00 00 00 fc fc fc fc fc fc fc fc fc fc fc
==================================================================

Re: [PATCH bpf] bpf: add check for invalid name in btf_name_valid_section()

[Eduard Zingerman]

On Thu, 2024-08-29 at 12:45 +0900, Jeongjun Park wrote:
> Alexei Starovoitov wrote:
> > 
> > On Fri, Aug 23, 2024 at 3:43 AM Jeongjun Park <aha310510@gmail.com> wrote:
> > > 
> > > If the length of the name string is 1 and the value of name[0] is NULL
> > > byte, an OOB vulnerability occurs in btf_name_valid_section() and the
> > > return value is true, so the invalid name passes the check.
> > > 
> > > To solve this, you need to check if the first position is NULL byte.
> > > 
> > > Fixes: bd70a8fb7ca4 ("bpf: Allow all printable characters in BTF DATASEC names")
> > > Signed-off-by: Jeongjun Park <aha310510@gmail.com>
> > > ---
> > >  kernel/bpf/btf.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > index 520f49f422fe..5c24ea1a65a4 100644
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -823,6 +823,9 @@ static bool btf_name_valid_section(const struct btf *btf, u32 offset)
> > >         const char *src = btf_str_by_offset(btf, offset);
> > >         const char *src_limit;
> > > 
> > > +       if (!*src)
> > > +               return false;
> > > +
> > 
> > We've talked about it. Quote:
> > "Pls add a selftest that demonstrates the issue
> > and produce a patch to fix just that."
> > 
> > length == 1 and name[0] = 0 is a hypothesis.
> > Demonstrate that such a scenario is possible then this patch will be
> > worth applying.
> > 
> > pw-bot: cr
> 
> Sorry for the omission, I still don't know how to write selftest.
> 
> But I can give you the C repro and KASAN log that trigger this vulnerability. 
> I would appreciate it if you could look at it and make a judgment.

I will prepare a test case.
Probably tomorrow.

Re: [PATCH bpf] bpf: add check for invalid name in btf_name_valid_section()

[Eduard Zingerman]

[-- Attachment #1: Type: text/plain, Size: 1314 bytes --]

On Wed, 2024-08-28 at 22:45 -0700, Eduard Zingerman wrote:

[...]

> I will prepare a test case.
> Probably tomorrow.

Please find test in the attachment. This test triggers KASAN error
report as in another attachment. (I enabled CONFIG_KASAN using
menuconfig on top of regular selftest config).

On Fri, Aug 23, 2024 at 3:43 AM Jeongjun Park <aha310510@gmail.com> wrote:

[...]

> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 520f49f422fe..5c24ea1a65a4 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -823,6 +823,9 @@ static bool btf_name_valid_section(const struct btf *btf, u32 offset)
>         const char *src = btf_str_by_offset(btf, offset);
>         const char *src_limit;
> 
> +       if (!*src)
> +               return false;
> +

I think that correct fix would be as follows:

---

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index edad152cee8e..d583d76fcace 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -820,7 +820,6 @@ static bool btf_name_valid_section(const struct btf *btf, u32 offset)
 
        /* set a limit on identifier length */
        src_limit = src + KSYM_NAME_LEN;
-       src++;
        while (*src && src < src_limit) {
                if (!isprint(*src))
                        return false;


[-- Attachment #2: bad-name-test.patch --]
[-- Type: text/x-patch, Size: 899 bytes --]

diff --git a/tools/testing/selftests/bpf/prog_tests/btf.c b/tools/testing/selftests/bpf/prog_tests/btf.c
index 00965a6e83bb..ea4d3f6d5de9 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf.c
@@ -3550,6 +3550,21 @@ static struct btf_raw_test raw_tests[] = {
 	},
 	BTF_STR_SEC("\0x\0?.foo bar:buz"),
 },
+{
+	.descr = "datasec: name '\\0' is not ok",
+	.raw_types = {
+		/* int */
+		BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4),  /* [1] */
+		/* VAR x */                                     /* [2] */
+		BTF_TYPE_ENC(1, BTF_INFO_ENC(BTF_KIND_VAR, 0, 0), 1),
+		BTF_VAR_STATIC,
+		/* DATASEC \0 */                                /* [3] */
+		BTF_TYPE_ENC(3, BTF_INFO_ENC(BTF_KIND_DATASEC, 0, 1), 4),
+		BTF_VAR_SECINFO_ENC(2, 0, 4),
+		BTF_END_RAW,
+	},
+	BTF_STR_SEC("\0x\0"),
+},
 {
 	.descr = "type name '?foo' is not ok",
 	.raw_types = {

[-- Attachment #3: bad-name-kasan-report.txt --]
[-- Type: text/plain, Size: 4997 bytes --]

[    7.025728] ==================================================================
[    7.025977] BUG: KASAN: slab-out-of-bounds in btf_datasec_check_meta+0x73c/0x7a0
[    7.026185] Read of size 1 at addr ffff888108eaa5d4 by task test_progs/172
[    7.026358] 
[    7.026430] CPU: 2 UID: 0 PID: 172 Comm: test_progs Tainted: G           OE      6.11.0-rc4-00225-g0673888b8469-dirty #23
[    7.026673] Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
[    7.026673] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-2.fc40 04/01/2014
[    7.026673] Call Trace:
[    7.026673]  <TASK>
[    7.026673]  dump_stack_lvl+0x5d/0x80
[    7.026673]  ? btf_datasec_check_meta+0x73c/0x7a0
[    7.026673]  print_report+0x171/0x502
[    7.026673]  ? btf_datasec_check_meta+0x73c/0x7a0
[    7.026673]  ? srso_alias_return_thunk+0x5/0xfbef5
[    7.026673]  ? __virt_addr_valid+0x20c/0x410
[    7.026673]  ? btf_datasec_check_meta+0x73c/0x7a0
[    7.026673]  kasan_report+0xda/0x1b0
[    7.026673]  ? btf_datasec_check_meta+0x73c/0x7a0
[    7.026673]  btf_datasec_check_meta+0x73c/0x7a0
[    7.026673]  btf_check_all_metas+0x32b/0xac0
[    7.026673]  btf_new_fd+0x6fb/0x2dc0
[    7.026673]  ? srso_alias_return_thunk+0x5/0xfbef5
[    7.026673]  ? cred_has_capability.isra.0+0x139/0x230
[    7.026673]  ? __pfx_cred_has_capability.isra.0+0x10/0x10
[    7.026673]  ? srso_alias_return_thunk+0x5/0xfbef5
[    7.026673]  ? __pfx_btf_new_fd+0x10/0x10
[    7.026673]  ? __pfx_lock_release+0x10/0x10
[    7.026673]  ? srso_alias_return_thunk+0x5/0xfbef5
[    7.026673]  ? security_capable+0x6f/0xb0
[    7.026673]  ? srso_alias_return_thunk+0x5/0xfbef5
[    7.026673]  __sys_bpf+0x1251/0x4650
[    7.026673]  ? srso_alias_return_thunk+0x5/0xfbef5
[    7.026673]  ? __pfx___sys_bpf+0x10/0x10
[    7.026673]  ? srso_alias_return_thunk+0x5/0xfbef5
[    7.026673]  ? find_held_lock+0x2d/0x110
[    7.026673]  ? srso_alias_return_thunk+0x5/0xfbef5
[    7.026673]  ? lock_release+0x45a/0x7a0
[    7.026673]  ? __pfx_lock_release+0x10/0x10
[    7.026673]  ? __pfx___rseq_handle_notify_resume+0x10/0x10
[    7.026673]  __x64_sys_bpf+0x7b/0xc0
[    7.026673]  ? srso_alias_return_thunk+0x5/0xfbef5
[    7.026673]  ? lockdep_hardirqs_on+0x7b/0x100
[    7.026673]  do_syscall_64+0x68/0x140
[    7.026673]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[    7.026673] RIP: 0033:0x7f9398d281dd
[    7.026673] Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 0b dc 0c 00 f7 d8 64 89 01 48
[    7.026673] RSP: 002b:00007ffddc11b488 EFLAGS: 00000206 ORIG_RAX: 0000000000000141
[    7.026673] RAX: ffffffffffffffda RBX: 00007ffddc11b9f8 RCX: 00007f9398d281dd
[    7.026673] RDX: 0000000000000028 RSI: 00007ffddc11b500 RDI: 0000000000000012
[    7.026673] RBP: 00007ffddc11b4a0 R08: 00007ffddc11b320 R09: 00007ffddc11b500
[    7.026673] R10: 0000000000000007 R11: 0000000000000206 R12: 0000000000000003
[    7.026673] R13: 0000000000000000 R14: 00007f93a0114000 R15: 0000000001090d90
[    7.026673]  </TASK>
[    7.026673] 
[    7.026673] Allocated by task 172:
[    7.026673]  kasan_save_stack+0x30/0x50
[    7.026673]  kasan_save_track+0x14/0x30
[    7.026673]  __kasan_kmalloc+0x8f/0xa0
[    7.026673]  __kmalloc_node_noprof+0x1ac/0x490
[    7.026673]  btf_new_fd+0x3e4/0x2dc0
[    7.026673]  __sys_bpf+0x1251/0x4650
[    7.026673]  __x64_sys_bpf+0x7b/0xc0
[    7.026673]  do_syscall_64+0x68/0x140
[    7.026673]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[    7.026673] 
[    7.026673] The buggy address belongs to the object at ffff888108eaa580
[    7.026673]  which belongs to the cache kmalloc-96 of size 96
[    7.026673] The buggy address is located 0 bytes to the right of
[    7.026673]  allocated 84-byte region [ffff888108eaa580, ffff888108eaa5d4)
[    7.026673] 
[    7.026673] The buggy address belongs to the physical page:
[    7.026673] page: refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x108eaa
[    7.026673] flags: 0x2fffe0000000000(node=0|zone=2|lastcpupid=0x7fff)
[    7.026673] page_type: 0xfdffffff(slab)
[    7.026673] raw: 02fffe0000000000 ffff888100042280 dead000000000122 0000000000000000
[    7.026673] raw: 0000000000000000 0000000080200020 00000001fdffffff 0000000000000000
[    7.026673] page dumped because: kasan: bad access detected
[    7.026673] 
[    7.026673] Memory state around the buggy address:
[    7.026673]  ffff888108eaa480: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
[    7.026673]  ffff888108eaa500: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
[    7.026673] >ffff888108eaa580: 00 00 00 00 00 00 00 00 00 00 04 fc fc fc fc fc
[    7.026673]                                                  ^
[    7.026673]  ffff888108eaa600: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[    7.026673]  ffff888108eaa680: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[    7.026673] ==================================================================

Re: [PATCH bpf] bpf: add check for invalid name in btf_name_valid_section()

[Jeongjun Park]

> Eduard Zingerman wrote:
> 
> On Wed, 2024-08-28 at 22:45 -0700, Eduard Zingerman wrote:
> 
> [...]
> 
>> I will prepare a test case.
>> Probably tomorrow.
> 
> Please find test in the attachment. This test triggers KASAN error
> report as in another attachment. (I enabled CONFIG_KASAN using
> menuconfig on top of regular selftest config).
> 

Thank you for writing the selftest for me.

> On Fri, Aug 23, 2024 at 3:43 AM Jeongjun Park <aha310510@gmail.com> wrote:
> 
> [...]
> 
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index 520f49f422fe..5c24ea1a65a4 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -823,6 +823,9 @@ static bool btf_name_valid_section(const struct btf *btf, u32 offset)
>>        const char *src = btf_str_by_offset(btf, offset);
>>        const char *src_limit;
>> 
>> +       if (!*src)
>> +               return false;
>> +
> 
> I think that correct fix would be as follows:
> 
> ---
> 
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index edad152cee8e..d583d76fcace 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -820,7 +820,6 @@ static bool btf_name_valid_section(const struct btf *btf, u32 offset)
> 
>        /* set a limit on identifier length */
>        src_limit = src + KSYM_NAME_LEN;
> -       src++;
>        while (*src && src < src_limit) {
>                if (!isprint(*src))
>                        return false;

However, this patch is logically flawed. 
It will return true for invalid names with 
length 1 and src[0] being NULL. So I think 
it's better to stick with the original patch.

> 
> <bad-name-test.patch>
> <bad-name-kasan-report.txt>

Re: [PATCH bpf] bpf: add check for invalid name in btf_name_valid_section()

[Eduard Zingerman]

On Fri, 2024-08-30 at 11:03 +0900, Jeongjun Park wrote:

[...]

> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index edad152cee8e..d583d76fcace 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -820,7 +820,6 @@ static bool btf_name_valid_section(const struct btf *btf, u32 offset)
> > 
> >        /* set a limit on identifier length */
> >        src_limit = src + KSYM_NAME_LEN;
> > -       src++;
> >        while (*src && src < src_limit) {
> >                if (!isprint(*src))
> >                        return false;
> 
> However, this patch is logically flawed. 
> It will return true for invalid names with 
> length 1 and src[0] being NULL. So I think 
> it's better to stick with the original patch.

Fair enough, however the isprint check should be done for the first character.
So the full fix is a combination :)

--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -818,9 +818,11 @@ static bool btf_name_valid_section(const struct btf *btf, u32 offset)
        const char *src = btf_str_by_offset(btf, offset);
        const char *src_limit;
 
+       if (!*src)
+               return false;
+
        /* set a limit on identifier length */
        src_limit = src + KSYM_NAME_LEN;
-       src++;
        while (*src && src < src_limit) {
                if (!isprint(*src))
                        return false;


And corresponding test cases (tools/testing/selftests/bpf/prog_tests/btf.c):

{
	.descr = "datasec: name with non-printable first char not is ok",
	.raw_types = {
		/* int */
		BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4),  /* [1] */
		/* VAR x */                                     /* [2] */
		BTF_TYPE_ENC(1, BTF_INFO_ENC(BTF_KIND_VAR, 0, 0), 1),
		BTF_VAR_STATIC,
		/* DATASEC ?.data */                            /* [3] */
		BTF_TYPE_ENC(3, BTF_INFO_ENC(BTF_KIND_DATASEC, 0, 1), 4),
		BTF_VAR_SECINFO_ENC(2, 0, 4),
		BTF_END_RAW,
	},
	BTF_STR_SEC("\0x\0\7foo"),
	.err_str = "Invalid name",
	.btf_load_err = true,
},{
	.descr = "datasec: name '\\0' is not ok",
	.raw_types = {
		/* int */
		BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4),  /* [1] */
		/* VAR x */                                     /* [2] */
		BTF_TYPE_ENC(1, BTF_INFO_ENC(BTF_KIND_VAR, 0, 0), 1),
		BTF_VAR_STATIC,
		/* DATASEC \0 */                                /* [3] */
		BTF_TYPE_ENC(3, BTF_INFO_ENC(BTF_KIND_DATASEC, 0, 1), 4),
		BTF_VAR_SECINFO_ENC(2, 0, 4),
		BTF_END_RAW,
	},
	BTF_STR_SEC("\0x\0"),
	.err_str = "Invalid name",
	.btf_load_err = true,
},

Could you please resend your patch as a patch-set fix + selftests update?

Re: [PATCH bpf] bpf: add check for invalid name in btf_name_valid_section()

[Jeongjun Park]

Eduard Zingerman wrote:
>
> On Fri, 2024-08-30 at 11:03 +0900, Jeongjun Park wrote:
>
> [...]
>
> > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > index edad152cee8e..d583d76fcace 100644
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -820,7 +820,6 @@ static bool btf_name_valid_section(const struct btf *btf, u32 offset)
> > >
> > >        /* set a limit on identifier length */
> > >        src_limit = src + KSYM_NAME_LEN;
> > > -       src++;
> > >        while (*src && src < src_limit) {
> > >                if (!isprint(*src))
> > >                        return false;
> >
> > However, this patch is logically flawed.
> > It will return true for invalid names with
> > length 1 and src[0] being NULL. So I think
> > it's better to stick with the original patch.
>
> Fair enough, however the isprint check should be done for the first character.
> So the full fix is a combination :)

So does that mean it's appropriate to add if(!isprint(*src)) instead
of if(!*src)?
As far as I know, the first character of name doesn't need isprint() check,
so if that's true, it would be appropriate to use isprint. Once this
is confirmed,
I'll send you a v2 patch that added selftest.

Regards,
Jeongjun Park

>
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -818,9 +818,11 @@ static bool btf_name_valid_section(const struct btf *btf, u32 offset)
>         const char *src = btf_str_by_offset(btf, offset);
>         const char *src_limit;
>
> +       if (!*src)
> +               return false;
> +
>         /* set a limit on identifier length */
>         src_limit = src + KSYM_NAME_LEN;
> -       src++;
>         while (*src && src < src_limit) {
>                 if (!isprint(*src))
>                         return false;
>
>
> And corresponding test cases (tools/testing/selftests/bpf/prog_tests/btf.c):
>
> {
>         .descr = "datasec: name with non-printable first char not is ok",
>         .raw_types = {
>                 /* int */
>                 BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4),  /* [1] */
>                 /* VAR x */                                     /* [2] */
>                 BTF_TYPE_ENC(1, BTF_INFO_ENC(BTF_KIND_VAR, 0, 0), 1),
>                 BTF_VAR_STATIC,
>                 /* DATASEC ?.data */                            /* [3] */
>                 BTF_TYPE_ENC(3, BTF_INFO_ENC(BTF_KIND_DATASEC, 0, 1), 4),
>                 BTF_VAR_SECINFO_ENC(2, 0, 4),
>                 BTF_END_RAW,
>         },
>         BTF_STR_SEC("\0x\0\7foo"),
>         .err_str = "Invalid name",
>         .btf_load_err = true,
> },{
>         .descr = "datasec: name '\\0' is not ok",
>         .raw_types = {
>                 /* int */
>                 BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4),  /* [1] */
>                 /* VAR x */                                     /* [2] */
>                 BTF_TYPE_ENC(1, BTF_INFO_ENC(BTF_KIND_VAR, 0, 0), 1),
>                 BTF_VAR_STATIC,
>                 /* DATASEC \0 */                                /* [3] */
>                 BTF_TYPE_ENC(3, BTF_INFO_ENC(BTF_KIND_DATASEC, 0, 1), 4),
>                 BTF_VAR_SECINFO_ENC(2, 0, 4),
>                 BTF_END_RAW,
>         },
>         BTF_STR_SEC("\0x\0"),
>         .err_str = "Invalid name",
>         .btf_load_err = true,
> },
>
> Could you please resend your patch as a patch-set fix + selftests update?
>

Re: [PATCH bpf] bpf: add check for invalid name in btf_name_valid_section()

[Eduard Zingerman]

On Fri, 2024-08-30 at 20:41 +0900, Jeongjun Park wrote:

[...]

> So does that mean it's appropriate to add if(!isprint(*src)) instead
> of if(!*src)?

I'd prefer to add "if (!*src) return false" and remove "src++" in
order to not repeat isprint().

> As far as I know, the first character of name doesn't need isprint() check,
> so if that's true, it would be appropriate to use isprint. Once this
> is confirmed,

The intent of the buggy commit [1] was to check that all characters in
a section name are printable. But I can't even check name for
printable characters these days :(

[1] bd70a8fb7ca4 ("bpf: Allow all printable characters in BTF DATASEC names")

[...]