code.ossystems Code Review - openembedded-core.git/blob

   1 From 23a2f61ffc6a656f136fa2044c0c3b8f79766779 Mon Sep 17 00:00:00 2001
   2 From: =?UTF-8?q?J=C3=A9r=C3=A9mie=20Galarneau?=
   3  <jeremie.galarneau@efficios.com>
   4 Date: Wed, 3 Mar 2021 18:52:19 -0500
   5 Subject: [PATCH 2/4] Fix: filter interpreter early-exits on uninitialized
   6  value
   7 MIME-Version: 1.0
   8 Content-Type: text/plain; charset=UTF-8
   9 Content-Transfer-Encoding: 8bit
  10
  11 I observed that syscall filtering on string arguments wouldn't work on
  12 my development machines, both running 5.11.2-arch1-1 (Arch Linux).
  13
  14 For instance, enabling the tracing of the `openat()` syscall with the
  15 'filename == "/proc/cpuinfo"' filter would not produce events even
  16 though matching events were present in another session that had no
  17 filtering active. The same problem occurred with `execve()`.
  18
  19 I tried a couple of kernel versions before (5.11.1 and 5.10.13, if
  20 memory serves me well) and I had the same problem. Meanwhile, I couldn't
  21 reproduce the problem on various Debian machines (the LTTng CI) nor on a
  22 fresh Ubuntu 20.04 with both the stock kernel and with an updated 5.11.2
  23 kernel.
  24
  25 I built the lttng-modules with the interpreter debugging printout and
  26 saw the following warning:
  27   LTTng: [debug bytecode in /home/jgalar/EfficiOS/src/lttng-modules/src/lttng-bytecode-interpreter.c:bytecode_interpret@1508] Bytecode warning: loading a NULL string.
  28
  29 After a shedload (yes, a _shed_load) of digging, I figured that the
  30 problem was hidden in plain sight near that logging statement.
  31
  32 In the `BYTECODE_OP_LOAD_FIELD_REF_USER_STRING` operation, the 'ax'
  33 register's 'user_str' is initialized with the stack value (the user
  34 space string's address in our case). However, a NULL check is performed
  35 against the register's 'str' member.
  36
  37 I initialy suspected that both members would be part of the same union
  38 and alias each-other, but they are actually contiguous in a structure.
  39
  40 On the unaffected machines, I could confirm that the `str` member was
  41 uninitialized to a non-zero value causing the condition to evaluate to
  42 false.
  43
  44 Francis Deslauriers reproduced the problem by initializing the
  45 interpreter stack to zero.
  46
  47 I am unsure of the exact kernel configuration option that reveals this
  48 issue on Arch Linux, but my kernel has the following option enabled:
  49
  50 CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL:
  51    Zero-initialize any stack variables that may be passed by reference
  52    and had not already been explicitly initialized. This is intended to
  53    eliminate all classes of uninitialized stack variable exploits and
  54    information exposures.
  55
  56 I have not tried to build without this enabled as, anyhow, this seems
  57 to be a legitimate issue.
  58
  59 I have spotted what appears to be an identical problem in
  60 `BYTECODE_OP_LOAD_FIELD_REF_USER_SEQUENCE` and corrected it. However,
  61 I have not exercised that code path.
  62
  63 The commit that introduced this problem is 5b4ad89.
  64
  65 The debug print-out of the `BYTECODE_OP_LOAD_FIELD_REF_USER_STRING`
  66 operation is modified to print the user string (truncated to 31 chars).
  67
  68 Upstream-status: backport
  69
  70 Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
  71 Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
  72 Change-Id: I2da3c31b9e3ce0e1b164cf3d2711c0893cbec273
  73 ---
  74  lttng-filter-interpreter.c | 41 ++++++++++++++++++++++++++++++++++----
  75  1 file changed, 37 insertions(+), 4 deletions(-)
  76
  77 diff --git a/lttng-filter-interpreter.c b/lttng-filter-interpreter.c
  78 index 5d572437..6e5a5139 100644
  79 --- a/lttng-filter-interpreter.c
  80 +++ b/lttng-filter-interpreter.c
  81 @@ -22,7 +22,7 @@ LTTNG_STACK_FRAME_NON_STANDARD(lttng_filter_interpret_bytecode);
  82   * to handle user-space read.
  83   */
  84  static
  85 -char get_char(struct estack_entry *reg, size_t offset)
  86 +char get_char(const struct estack_entry *reg, size_t offset)
  87  {
  88         if (unlikely(offset >= reg->u.s.seq_len))
  89                 return '\0';
  90 @@ -593,6 +593,39 @@ end:
  91         return ret;
  92  }
  93
  94 +#ifdef DEBUG
  95 +
  96 +#define DBG_USER_STR_CUTOFF 32
  97 +
  98 +/*
  99 + * In debug mode, print user string (truncated, if necessary).
 100 + */
 101 +static inline
 102 +void dbg_load_ref_user_str_printk(const struct estack_entry *user_str_reg)
 103 +{
 104 +       size_t pos = 0;
 105 +       char last_char;
 106 +       char user_str[DBG_USER_STR_CUTOFF];
 107 +
 108 +       pagefault_disable();
 109 +       do {
 110 +               last_char = get_char(user_str_reg, pos);
 111 +               user_str[pos] = last_char;
 112 +               pos++;
 113 +       } while (last_char != '\0' && pos < sizeof(user_str));
 114 +       pagefault_enable();
 115 +
 116 +       user_str[sizeof(user_str) - 1] = '\0';
 117 +       dbg_printk("load field ref user string: '%s%s'\n", user_str,
 118 +               last_char != '\0' ? "[...]" : "");
 119 +}
 120 +#else
 121 +static inline
 122 +void dbg_load_ref_user_str_printk(const struct estack_entry *user_str_reg)
 123 +{
 124 +}
 125 +#endif
 126 +
 127  /*
 128   * Return 0 (discard), or raise the 0x1 flag (log event).
 129   * Currently, other flags are kept for future extensions and have no
 130 @@ -1313,7 +1346,7 @@ uint64_t lttng_filter_interpret_bytecode(void *filter_data,
 131                         estack_push(stack, top, ax, bx);
 132                         estack_ax(stack, top)->u.s.user_str =
 133                                 *(const char * const *) &filter_stack_data[ref->offset];
 134 -                       if (unlikely(!estack_ax(stack, top)->u.s.str)) {
 135 +                       if (unlikely(!estack_ax(stack, top)->u.s.user_str)) {
 136                                 dbg_printk("Filter warning: loading a NULL string.\n");
 137                                 ret = -EINVAL;
 138                                 goto end;
 139 @@ -1322,7 +1355,7 @@ uint64_t lttng_filter_interpret_bytecode(void *filter_data,
 140                         estack_ax(stack, top)->u.s.literal_type =
 141                                 ESTACK_STRING_LITERAL_TYPE_NONE;
 142                         estack_ax(stack, top)->u.s.user = 1;
 143 -                       dbg_printk("ref load string %s\n", estack_ax(stack, top)->u.s.str);
 144 +                       dbg_load_ref_user_str_printk(estack_ax(stack, top));
 145                         next_pc += sizeof(struct load_op) + sizeof(struct field_ref);
 146                         PO;
 147                 }
 148 @@ -1340,7 +1373,7 @@ uint64_t lttng_filter_interpret_bytecode(void *filter_data,
 149                         estack_ax(stack, top)->u.s.user_str =
 150                                 *(const char **) (&filter_stack_data[ref->offset
 151                                                                 + sizeof(unsigned long)]);
 152 -                       if (unlikely(!estack_ax(stack, top)->u.s.str)) {
 153 +                       if (unlikely(!estack_ax(stack, top)->u.s.user_str)) {
 154                                 dbg_printk("Filter warning: loading a NULL sequence.\n");
 155                                 ret = -EINVAL;
 156                                 goto end;
 157 --
 158 2.19.1
 159