From a62977cac65f63f20625dd501282de1ea0156ee0 Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=A9mie=20Galarneau?= Date: Thu, 5 Oct 2023 17:02:57 -0400 Subject: [PATCH] Fix: bytecode validator: oops during validation of immediate string MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Issue observed -------------- Running Linux 6.5.5, lttng-modules @ 6be48c9f, all built with gcc 13.2.1, I got a 'BUG' in dmesg while enabling the following event rule: $ lttng enable-event --kernel --syscall --channel chanK --all --filter '$ctx.procname == "UST reg*"' The relevant parts of the 'BUG' output follow: [ +0.715480] detected buffer overflow in strnlen [ +0.000001] kernel BUG at lib/string_helpers.c:1031! [ +0.000008] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI [ +0.000003] CPU: 2 PID: 157174 Comm: Client manageme Tainted: G S U OE 6.5.5-arch1-1 #1 d82a0f532dd8cfe67d5795c1738d9c01059a0c62 [ +0.000001] RIP: 0010:fortify_panic+0x13/0x20 [ +0.000006] Code: 41 5d c3 cc cc cc cc 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa 48 89 fe 48 c7 c7 90 22 c8 86 e8 3d aa b1 ff <0f> 0b 66 66 2e 0f 1f 84 00 00 00 00 00 90 90 90 90 90 90 90 90 90 [ +0.000002] RSP: 0018:ffffa7c7c106f918 EFLAGS: 00010246 [ +0.000002] RAX: 0000000000000023 RBX: 000000000000000b RCX: 0000000000000000 [ +0.000002] RDX: 0000000000000000 RSI: ffff92766e4a16c0 RDI: ffff92766e4a16c0 [ +0.000001] RBP: 0000000000000000 R08: 0000000000000000 R09: ffffa7c7c106f7c0 [ +0.000001] R10: 0000000000000003 R11: ffffffff874ca068 R12: ffff927618202480 [ +0.000001] R13: ffff9276182024d2 R14: ffff927453999c08 R15: ffff9273dc7aa478 [ +0.000001] FS: 00007f06553f9680(0000) GS:ffff92766e480000(0000) knlGS:0000000000000000 [ +0.000002] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ +0.000002] CR2: 0000556d54eceaa8 CR3: 00000001ad9de002 CR4: 00000000003706e0 [ +0.000001] Call Trace: [ +0.000002] [ +0.000002] ? die+0x36/0x90 [ +0.000004] ? do_trap+0xda/0x100 [ +0.000003] ? fortify_panic+0x13/0x20 [ +0.000002] ? do_error_trap+0x6a/0x90 [ +0.000002] ? fortify_panic+0x13/0x20 [ +0.000002] ? exc_invalid_op+0x50/0x70 [ +0.000003] ? fortify_panic+0x13/0x20 [ +0.000002] ? asm_exc_invalid_op+0x1a/0x20 [ +0.000005] ? fortify_panic+0x13/0x20 [ +0.000002] ? fortify_panic+0x13/0x20 [ +0.000003] bytecode_validate_overflow+0x155/0x1f0 [lttng_tracer 759e3e4fee0e774ef575e93b67e8dc7955d0c2c2] [ +0.000330] lttng_bytecode_validate_load+0x32/0x1e0 [lttng_tracer 759e3e4fee0e774ef575e93b67e8dc7955d0c2c2] [ +0.000183] lttng_enabler_link_bytecode+0x135/0x5a0 [lttng_tracer 759e3e4fee0e774ef575e93b67e8dc7955d0c2c2] [ +0.000132] lttng_sync_event_list+0xef/0x650 [lttng_tracer 759e3e4fee0e774ef575e93b67e8dc7955d0c2c2] [ +0.000123] ? __wake_up_common+0x73/0x180 [ +0.000004] lttng_session_enable+0x3e/0x130 [lttng_tracer 759e3e4fee0e774ef575e93b67e8dc7955d0c2c2] [ +0.000121] lttng_session_ioctl+0x5db/0x720 [lttng_tracer 759e3e4fee0e774ef575e93b67e8dc7955d0c2c2] [ +0.000120] ? __slab_free+0xf1/0x330 [ +0.000004] ? __scm_recv_common.isra.0+0x144/0x180 [ +0.000004] ? unix_stream_read_generic+0x233/0xb60 [ +0.000006] __x64_sys_ioctl+0x94/0xd0 [ +0.000004] do_syscall_64+0x5d/0x90 [ +0.000004] ? switch_fpu_return+0x50/0xe0 [ +0.000004] ? exit_to_user_mode_prepare+0x132/0x1e0 [ +0.000003] ? syscall_exit_to_user_mode+0x2b/0x40 [ +0.000002] ? do_syscall_64+0x6c/0x90 [ +0.000003] ? do_syscall_64+0x6c/0x90 [ +0.000002] ? do_syscall_64+0x6c/0x90 [ +0.000002] ? do_syscall_64+0x6c/0x90 [ +0.000002] ? syscall_exit_to_user_mode+0x2b/0x40 [ +0.000002] ? do_syscall_64+0x6c/0x90 [ +0.000002] ? do_syscall_64+0x6c/0x90 [ +0.000002] ? do_syscall_64+0x6c/0x90 [ +0.000002] ? do_syscall_64+0x6c/0x90 [ +0.000002] ? exc_page_fault+0x7f/0x180 [ +0.000003] entry_SYSCALL_64_after_hwframe+0x6e/0xd8 Cause ----- `struct load_op` has a trailing 0-length array `data` member that is used to refer, in the context of BYTECODE_OP_LOAD_STAR_GLOB_STRING, to an immediate string operand that follows it. During the validation of a filtering bytecode, strnlen is properly used to determine the size of the immediate string operand, with a `maxlen` parameter that is used to ensure the string operand is contained within the bytecode (see lttng-bytecode-validator.c:434). However, recent KSPP-related changes have enabled additional overrun checks when statically-sized and flexible arrays are used. Those are enabled when the kernel is built with CONFIG_UBSAN_BOUNDS and/or CONFIG_FORTIFY_SOURCE configured. The KBUILD CFLAGS now contain `-fstrict-flex-arrays=3`, which is recognized by gcc 13+[1] and allows proper coverage of dynamically sized trailing arrays when those configuration options are used. With those validations in place, the kernel assumes that the `data` array is truly of length 0 and it BUGs to warn of an invalid access. The commit linked above contains a number of links explaining the rationale for transitioning uses of the trailing zero-length arrays (a gcc extension) to C99 flexible array members (FAM). This was discussed at this year's GNU Cauldron [2]. Solution -------- Uses of zero-length arrays (`foo[0]`) are replaced by flexible array members (`foo[]`). The only cases that are left untouched are those where the zero-length array is used to indicate the end of a structure (i.e. it doesn't indicate that a variable number of elements follow), see the `metadata_packet_header`, `metadata_record_header`, `event_notifier_packet_header`, and `event_notifier_record_header` structures. It may be desirable to use the new `counted_by` attribute for some of those in the future (`lttng_kernel_abi_filter_bytecode`, `lttng_kernel_abi_capture_bytecode`, and `bytecode_runtime`) [3]. Note ---- While this is tagged as a memory handling 'fix', it has no security implication as far as I can tell. The accesses that are flagged by the new validations were valid. This merely allows the runtime validations to understand the memory layout properly. [1] https://github.com/torvalds/linux/commit/df8fc4e934c12b906d08050d7779f292b9c5c6b5 [2] https://gcc.gnu.org/wiki/cauldron2023talks?action=AttachFile&do=get&target=Most-wanted+Security+Features+in+GCC+for+Linux+Kernel.pdf [3] https://lwn.net/Articles/930943/ Signed-off-by: Jérémie Galarneau Signed-off-by: Mathieu Desnoyers Change-Id: Id39b101aaafe68f8fae6b86cd61806cba8cb1e6a --- include/lttng/abi.h | 4 ++-- include/lttng/bytecode.h | 6 +----- include/lttng/lttng-bytecode.h | 2 +- src/lttng-tracepoint.c | 2 +- 4 files changed, 5 insertions(+), 9 deletions(-) diff --git a/include/lttng/abi.h b/include/lttng/abi.h index 02bcefae..f8af1dd0 100644 --- a/include/lttng/abi.h +++ b/include/lttng/abi.h @@ -325,7 +325,7 @@ struct lttng_kernel_abi_filter_bytecode { uint32_t len; uint32_t reloc_offset; uint64_t seqnum; - char data[0]; + char data[]; } __attribute__((packed)); #define LTTNG_KERNEL_ABI_CAPTURE_BYTECODE_MAX_LEN 65536 @@ -333,7 +333,7 @@ struct lttng_kernel_abi_capture_bytecode { uint32_t len; uint32_t reloc_offset; uint64_t seqnum; - char data[0]; + char data[]; } __attribute__((packed)); enum lttng_kernel_abi_tracker_type { diff --git a/include/lttng/bytecode.h b/include/lttng/bytecode.h index 6bb20b83..89f8d0fc 100644 --- a/include/lttng/bytecode.h +++ b/include/lttng/bytecode.h @@ -40,10 +40,6 @@ struct literal_double { double v; } __attribute__((packed)); -struct literal_string { - char string[0]; -} __attribute__((packed)); - enum bytecode_op { BYTECODE_OP_UNKNOWN = 0, @@ -196,7 +192,7 @@ typedef uint8_t bytecode_opcode_t; struct load_op { bytecode_opcode_t op; - char data[0]; + char data[]; /* data to load. Size known by enum filter_opcode and null-term char. */ } __attribute__((packed)); diff --git a/include/lttng/lttng-bytecode.h b/include/lttng/lttng-bytecode.h index 5bff5b00..f104b0eb 100644 --- a/include/lttng/lttng-bytecode.h +++ b/include/lttng/lttng-bytecode.h @@ -42,7 +42,7 @@ struct bytecode_runtime { size_t data_alloc_len; char *data; uint16_t len; - char code[0]; + char code[]; }; enum entry_type { diff --git a/src/lttng-tracepoint.c b/src/lttng-tracepoint.c index 53828104..35aa07eb 100644 --- a/src/lttng-tracepoint.c +++ b/src/lttng-tracepoint.c @@ -41,7 +41,7 @@ struct tracepoint_entry { struct tracepoint *tp; int refcount; struct list_head probes; - char name[0]; + char name[]; }; struct lttng_tp_probe { -- 2.34.1