[PATCH v5 08/23] perf trace: Bounds check augmented arguments before reading them

[Ian Rogers]

Augmented arguments are length prefixed payloads that the BPF augmenter
appends to a sample: a struct augmented_arg header holding the payload
length, followed by the payload. arg->augmented.args points at the next
unconsumed one and arg->augmented.size says how many bytes are left.

Both the length and the number of bytes remaining come from the sample,
but syscall_arg__scnprintf_buf() takes the length on trust and loops
over it with no bound at all, so a bogus value walks straight off the
end of the static argbuf that syscall__augmented_args() copies into:

  #0 0x73104f in dump_stack debug.c:366
  #1 0x7310c5 in sighandler_dump_stack debug.c:378
  #2 0x7f749ea66fb0 in __restore_rt libc.so.6[19fb0]
  #3 0x4c0fa0 in syscall_arg__scnprintf_buf builtin-trace.c:1955
  #4 0x4c2f3d in syscall_arg_fmt__scnprintf_val builtin-trace.c:2632
  #5 0x4c33ae in syscall__scnprintf_args builtin-trace.c:2722
  #6 0x4c43d3 in trace__sys_enter builtin-trace.c:3094
  #7 0x4c7865 in trace__handle_event builtin-trace.c:4013

syscall_arg__scnprintf_augmented_string() likewise prints the payload
with "%.*s" and a length it has not checked, and both dereference the
header before testing it for NULL.

btf_struct_scnprintf() already gets this right. Lift its check into a
shared syscall_arg__augmented_args_valid() in beauty.h, which verifies
that a whole header is present and that the payload it advertises fits
in what is left, taking a @need argument for callers that go on to read
a fixed sized type, and use it at each of these sites.

An unusable payload now makes syscall_arg__scnprintf_filename() fall
back to the vfs_getname probe or the raw pointer, as it does when there
is no augmented data at all, rather than printing nothing.

Reported-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
Closes: https://lore.kernel.org/linux-perf-users/arJ-gpzqOHk-gF8T@x2/
Assisted-by: Antigravity:gemini-3.1-pro
Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
---
 tools/perf/builtin-trace.c       | 29 ++++++++++++++-------
 tools/perf/trace/beauty/beauty.h | 43 ++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 9 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 6ffd4a0718ca..eeaaab44016c 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -1139,14 +1139,12 @@ static size_t btf_struct_scnprintf(const struct btf_type *type, struct btf *btf,
 	LIBBPF_OPTS(btf_dump_opts, dump_opts);
 	LIBBPF_OPTS(btf_dump_type_data_opts, dump_data_opts);
 
-	if (arg == NULL || arg->augmented.args == NULL || arg->augmented.size < (int)sizeof(*augmented_arg) ||
+	/* btf_dump__dump_type_data() below reads type->size bytes of the payload. */
+	if (!syscall_arg__augmented_args_valid(arg, type->size) ||
 	    arg->fmt == NULL || !arg->fmt->from_user)
 		return 0;
 
 	augmented_arg = arg->augmented.args;
-	if (augmented_arg->size <= 0 || augmented_arg->size > arg->augmented.size - (int)sizeof(*augmented_arg) ||
-	    (size_t)augmented_arg->size < type->size)
-		return 0;
 
 	dump_data_opts.compact	  = true;
 	dump_data_opts.skip_names = !arg->trace->show_arg_names;
@@ -1904,12 +1902,18 @@ static void thread__set_filename_pos(struct thread *thread, const char *bf,
 static size_t syscall_arg__scnprintf_augmented_string(struct syscall_arg *arg, char *bf, size_t size)
 {
 	struct augmented_arg *augmented_arg = arg->augmented.args;
-	size_t printed = scnprintf(bf, size, "\"%.*s\"", augmented_arg->size, augmented_arg->value);
+	size_t printed;
+	int consumed;
+
+	if (!syscall_arg__augmented_args_valid(arg, 0))
+		return 0;
+
+	printed = scnprintf(bf, size, "\"%.*s\"", augmented_arg->size, augmented_arg->value);
 	/*
 	 * So that the next arg with a payload can consume its augmented arg, i.e. for rename* syscalls
 	 * we would have two strings, each prefixed by its size.
 	 */
-	int consumed = sizeof(*augmented_arg) + augmented_arg->size;
+	consumed = sizeof(*augmented_arg) + augmented_arg->size;
 
 	arg->augmented.args = ((void *)arg->augmented.args) + consumed;
 	arg->augmented.size -= consumed;
@@ -1922,7 +1926,12 @@ static size_t syscall_arg__scnprintf_filename(char *bf, size_t size,
 {
 	unsigned long ptr = arg->val;
 
-	if (arg->augmented.args)
+	/*
+	 * A truncated or otherwise unusable augmented payload is no better than
+	 * not having one at all, so test it here and let the pointer or the
+	 * vfs_getname probe name the file instead of printing nothing.
+	 */
+	if (syscall_arg__augmented_args_valid(arg, 0))
 		return syscall_arg__scnprintf_augmented_string(arg, bf, size);
 
 	if (!arg->trace->vfs_getname)
@@ -1938,13 +1947,15 @@ static size_t syscall_arg__scnprintf_filename(char *bf, size_t size,
 static size_t syscall_arg__scnprintf_buf(char *bf, size_t size, struct syscall_arg *arg)
 {
 	struct augmented_arg *augmented_arg = arg->augmented.args;
-	unsigned char *orig = (unsigned char *)augmented_arg->value;
 	size_t printed = 0;
+	unsigned char *orig;
 	int consumed;
 
-	if (augmented_arg == NULL)
+	if (!syscall_arg__augmented_args_valid(arg, 0))
 		return 0;
 
+	orig = (unsigned char *)augmented_arg->value;
+
 	for (int j = 0; j < augmented_arg->size; ++j) {
 		bool control_char = orig[j] <= MAX_CONTROL_CHAR || orig[j] >= MAX_ASCII;
 		/* print control characters (0~31 and 127), and non-ascii characters in \(digits) */
diff --git a/tools/perf/trace/beauty/beauty.h b/tools/perf/trace/beauty/beauty.h
index 0f4801c61a5b..71f93372c34e 100644
--- a/tools/perf/trace/beauty/beauty.h
+++ b/tools/perf/trace/beauty/beauty.h
@@ -124,6 +124,49 @@ struct syscall_arg {
 
 unsigned long syscall_arg__val(struct syscall_arg *arg, u8 idx);
 
+/**
+ * syscall_arg__augmented_args_valid: can the next augmented arg be read?
+ *
+ * Augmented arguments are a series of length prefixed payloads that the BPF
+ * augmenter appends to a sample, with arg->augmented.args pointing at the next
+ * unconsumed one and arg->augmented.size saying how many bytes are left.
+ *
+ * Both the remaining byte count and the length prefix come from the sample, so
+ * neither can be trusted: a sample that carries no augmented arguments at all
+ * can still have a few bytes of ring buffer padding after the tracepoint
+ * payload, and those bytes then look like a struct augmented_arg holding an
+ * arbitrary length.
+ *
+ * Check that a whole header is present and that the payload it advertises fits
+ * in what is left, so that a beautifier reading augmented_arg->value stays
+ * inside the sample.
+ *
+ * @need: how many bytes of payload the caller goes on to read, zero when it is
+ * driven by the length prefix itself rather than by a fixed sized type.
+ */
+static inline bool syscall_arg__augmented_args_valid(struct syscall_arg *arg, size_t need)
+{
+	const struct augmented_arg *augmented_arg;
+
+	if (arg == NULL || arg->augmented.args == NULL)
+		return false;
+
+	/* The header has to be there before its size field can be read. */
+	if (arg->augmented.size < (int)sizeof(*augmented_arg))
+		return false;
+
+	augmented_arg = arg->augmented.args;
+
+	/* A negative length would make the bounds below meaningless. */
+	if (augmented_arg->size < 0)
+		return false;
+
+	if (augmented_arg->size > arg->augmented.size - (int)sizeof(*augmented_arg))
+		return false;
+
+	return (size_t)augmented_arg->size >= need;
+}
+
 size_t syscall_arg__scnprintf_strarray_flags(char *bf, size_t size, struct syscall_arg *arg);
 #define SCA_STRARRAY_FLAGS syscall_arg__scnprintf_strarray_flags
 
-- 
2.56.0.rc1.315.gc6ed9934b7-goog