From 23a2f61ffc6a656f136fa2044c0c3b8f79766779 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9mie=20Galarneau?= Date: Wed, 3 Mar 2021 18:52:19 -0500 Subject: [PATCH 2/4] Fix: filter interpreter early-exits on uninitialized value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I observed that syscall filtering on string arguments wouldn't work on my development machines, both running 5.11.2-arch1-1 (Arch Linux). For instance, enabling the tracing of the `openat()` syscall with the 'filename == "/proc/cpuinfo"' filter would not produce events even though matching events were present in another session that had no filtering active. The same problem occurred with `execve()`. I tried a couple of kernel versions before (5.11.1 and 5.10.13, if memory serves me well) and I had the same problem. Meanwhile, I couldn't reproduce the problem on various Debian machines (the LTTng CI) nor on a fresh Ubuntu 20.04 with both the stock kernel and with an updated 5.11.2 kernel. I built the lttng-modules with the interpreter debugging printout and saw the following warning: LTTng: [debug bytecode in /home/jgalar/EfficiOS/src/lttng-modules/src/lttng-bytecode-interpreter.c:bytecode_interpret@1508] Bytecode warning: loading a NULL string. After a shedload (yes, a _shed_load) of digging, I figured that the problem was hidden in plain sight near that logging statement. In the `BYTECODE_OP_LOAD_FIELD_REF_USER_STRING` operation, the 'ax' register's 'user_str' is initialized with the stack value (the user space string's address in our case). However, a NULL check is performed against the register's 'str' member. I initialy suspected that both members would be part of the same union and alias each-other, but they are actually contiguous in a structure. On the unaffected machines, I could confirm that the `str` member was uninitialized to a non-zero value causing the condition to evaluate to false. Francis Deslauriers reproduced the problem by initializing the interpreter stack to zero. I am unsure of the exact kernel configuration option that reveals this issue on Arch Linux, but my kernel has the following option enabled: CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL: Zero-initialize any stack variables that may be passed by reference and had not already been explicitly initialized. This is intended to eliminate all classes of uninitialized stack variable exploits and information exposures. I have not tried to build without this enabled as, anyhow, this seems to be a legitimate issue. I have spotted what appears to be an identical problem in `BYTECODE_OP_LOAD_FIELD_REF_USER_SEQUENCE` and corrected it. However, I have not exercised that code path. The commit that introduced this problem is 5b4ad89. The debug print-out of the `BYTECODE_OP_LOAD_FIELD_REF_USER_STRING` operation is modified to print the user string (truncated to 31 chars). Upstream-status: backport Signed-off-by: Jérémie Galarneau Signed-off-by: Mathieu Desnoyers Change-Id: I2da3c31b9e3ce0e1b164cf3d2711c0893cbec273 --- lttng-filter-interpreter.c | 41 ++++++++++++++++++++++++++++++++++---- 1 file changed, 37 insertions(+), 4 deletions(-) diff --git a/lttng-filter-interpreter.c b/lttng-filter-interpreter.c index 5d572437..6e5a5139 100644 --- a/lttng-filter-interpreter.c +++ b/lttng-filter-interpreter.c @@ -22,7 +22,7 @@ LTTNG_STACK_FRAME_NON_STANDARD(lttng_filter_interpret_bytecode); * to handle user-space read. */ static -char get_char(struct estack_entry *reg, size_t offset) +char get_char(const struct estack_entry *reg, size_t offset) { if (unlikely(offset >= reg->u.s.seq_len)) return '\0'; @@ -593,6 +593,39 @@ end: return ret; } +#ifdef DEBUG + +#define DBG_USER_STR_CUTOFF 32 + +/* + * In debug mode, print user string (truncated, if necessary). + */ +static inline +void dbg_load_ref_user_str_printk(const struct estack_entry *user_str_reg) +{ + size_t pos = 0; + char last_char; + char user_str[DBG_USER_STR_CUTOFF]; + + pagefault_disable(); + do { + last_char = get_char(user_str_reg, pos); + user_str[pos] = last_char; + pos++; + } while (last_char != '\0' && pos < sizeof(user_str)); + pagefault_enable(); + + user_str[sizeof(user_str) - 1] = '\0'; + dbg_printk("load field ref user string: '%s%s'\n", user_str, + last_char != '\0' ? "[...]" : ""); +} +#else +static inline +void dbg_load_ref_user_str_printk(const struct estack_entry *user_str_reg) +{ +} +#endif + /* * Return 0 (discard), or raise the 0x1 flag (log event). * Currently, other flags are kept for future extensions and have no @@ -1313,7 +1346,7 @@ uint64_t lttng_filter_interpret_bytecode(void *filter_data, estack_push(stack, top, ax, bx); estack_ax(stack, top)->u.s.user_str = *(const char * const *) &filter_stack_data[ref->offset]; - if (unlikely(!estack_ax(stack, top)->u.s.str)) { + if (unlikely(!estack_ax(stack, top)->u.s.user_str)) { dbg_printk("Filter warning: loading a NULL string.\n"); ret = -EINVAL; goto end; @@ -1322,7 +1355,7 @@ uint64_t lttng_filter_interpret_bytecode(void *filter_data, estack_ax(stack, top)->u.s.literal_type = ESTACK_STRING_LITERAL_TYPE_NONE; estack_ax(stack, top)->u.s.user = 1; - dbg_printk("ref load string %s\n", estack_ax(stack, top)->u.s.str); + dbg_load_ref_user_str_printk(estack_ax(stack, top)); next_pc += sizeof(struct load_op) + sizeof(struct field_ref); PO; } @@ -1340,7 +1373,7 @@ uint64_t lttng_filter_interpret_bytecode(void *filter_data, estack_ax(stack, top)->u.s.user_str = *(const char **) (&filter_stack_data[ref->offset + sizeof(unsigned long)]); - if (unlikely(!estack_ax(stack, top)->u.s.str)) { + if (unlikely(!estack_ax(stack, top)->u.s.user_str)) { dbg_printk("Filter warning: loading a NULL sequence.\n"); ret = -EINVAL; goto end; -- 2.19.1