From acbe9250b563528ba81088e3589847a2ab75d2f5 Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=A9mie=20Galarneau?= Date: Wed, 3 Mar 2021 18:52:19 -0500 Subject: [PATCH] Fix: filter interpreter early-exits on uninitialized value MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 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). Signed-off-by: Jérémie Galarneau Signed-off-by: Mathieu Desnoyers Change-Id: I2da3c31b9e3ce0e1b164cf3d2711c0893cbec273 --- src/lttng-bytecode-interpreter.c | 41 ++++++++++++++++++++++++++++---- 1 file changed, 37 insertions(+), 4 deletions(-) diff --git a/src/lttng-bytecode-interpreter.c b/src/lttng-bytecode-interpreter.c index e183e330..153aeee6 100644 --- a/src/lttng-bytecode-interpreter.c +++ b/src/lttng-bytecode-interpreter.c @@ -20,7 +20,7 @@ * 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'; @@ -711,6 +711,39 @@ again: return LTTNG_INTERPRETER_RECORD_FLAG; } +#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 @@ -1504,7 +1537,7 @@ uint64_t bytecode_interpret(void *interpreter_data, estack_push(stack, top, ax, bx, ax_t, bx_t); estack_ax(stack, top)->u.s.user_str = *(const char * const *) &interpreter_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("Bytecode warning: loading a NULL string.\n"); ret = -EINVAL; goto end; @@ -1514,7 +1547,7 @@ uint64_t bytecode_interpret(void *interpreter_data, ESTACK_STRING_LITERAL_TYPE_NONE; estack_ax(stack, top)->u.s.user = 1; estack_ax(stack, top)->type = REG_STRING; - 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; } @@ -1532,7 +1565,7 @@ uint64_t bytecode_interpret(void *interpreter_data, estack_ax(stack, top)->u.s.user_str = *(const char **) (&interpreter_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("Bytecode warning: loading a NULL sequence.\n"); ret = -EINVAL; goto end; -- 2.34.1