Fix: filter interpreter early-exits on uninitialized value

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 <jeremie.galarneau@efficios.com>
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Change-Id: I2da3c31b9e3ce0e1b164cf3d2711c0893cbec273

diff --git a/src/lttng-bytecode-interpreter.c b/src/lttng-bytecode-interpreter.c
index e183e330041a7d1e9883567477d41401c52b9566..153aeee656190330e34ce582b434b51a24883b99 100644 (file)
--- 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;