[PATCH v5 09/23] perf trace: Bound the fixed size augmented argument beautifiers

[Ian Rogers]

The timespec, sockaddr and perf_event_attr beautifiers read a fixed
sized type out of the augmented payload without checking that one is
there. The payload is only as long as the augmenter made it, which for
a sockaddr is the addrlen the tracee passed and so can be as little as
two bytes, and which for a perf_event_attr is the attr size the tracee
chose.

Gate each of them on syscall_arg__augmented_args_valid() with the size
it needs before its first read, so that a payload too short for the
type leaves the raw pointer printed instead, as happens when there is no
augmented data at all.

Two reads that are unbounded for reasons other than the header go with
them:

  - perf_event_attr__fprintf() always reads a whole struct
    perf_event_attr, so an attr from an older version is read past its
    end. Copy the payload into a zero padded local instead of reading it
    in place, which also replaces the local copy that was only made to
    fix up a zero size field.

  - af_local__scnprintf() prints sun_path as a plain string, but the
    payload holds only addrlen bytes and the path need not be NUL
    terminated. Pass the payload length to the address family printers
    so the path is printed with a bound, and so that the AF_INET and
    AF_INET6 printers do not read a sockaddr_in or sockaddr_in6 out of
    a payload too short to hold one.

Assisted-by: Antigravity:gemini-3.1-pro
Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
---
 tools/perf/trace/beauty/perf_event_open.c | 38 +++++++++-------
 tools/perf/trace/beauty/sockaddr.c        | 55 ++++++++++++++++++-----
 tools/perf/trace/beauty/timespec.c        |  2 +-
 3 files changed, 67 insertions(+), 28 deletions(-)

diff --git a/tools/perf/trace/beauty/perf_event_open.c b/tools/perf/trace/beauty/perf_event_open.c
index 6315b46bcdf0..fba8993f3b93 100644
--- a/tools/perf/trace/beauty/perf_event_open.c
+++ b/tools/perf/trace/beauty/perf_event_open.c
@@ -81,33 +81,39 @@ static size_t perf_event_attr___scnprintf(struct perf_event_attr *attr, char *bf
 
 static size_t syscall_arg__scnprintf_augmented_perf_event_attr(struct syscall_arg *arg, char *bf, size_t size)
 {
-	struct perf_event_attr *attr = (void *)arg->augmented.args->value;
+	const struct augmented_arg *augmented_arg = arg->augmented.args;
 	struct perf_event_attr local_attr;
+	size_t copied = (size_t)augmented_arg->size;
 
 	/*
-	 * augmented_raw_syscalls.bpf.c (shipped with perf) copies
-	 * PERF_ATTR_SIZE_VER0 bytes when the tracee passes size=0,
-	 * but leaves the size field as 0.  The payload size is
-	 * guaranteed by perf's own BPF program, not externally
-	 * controllable.  Copy to a local so we can fix up size
-	 * without writing to the potentially read-only augmented
-	 * args buffer.
+	 * The tracee picks attr.size, so the payload that
+	 * augmented_raw_syscalls.bpf.c copied can be anything from
+	 * PERF_ATTR_SIZE_VER0 up to the size this build of perf knows about,
+	 * while perf_event_attr__fprintf() below always reads a whole struct
+	 * perf_event_attr. Work on a zero padded local copy so that a payload
+	 * from an older attr is not read past its end, which also lets the size
+	 * be fixed up without writing to the augmented args buffer.
 	 */
-	if (!attr->size) {
-		memcpy(&local_attr, attr, PERF_ATTR_SIZE_VER0);
-		memset((void *)&local_attr + PERF_ATTR_SIZE_VER0, 0,
-		       sizeof(local_attr) - PERF_ATTR_SIZE_VER0);
+	if (copied > sizeof(local_attr))
+		copied = sizeof(local_attr);
+
+	memcpy(&local_attr, augmented_arg->value, copied);
+	memset((void *)&local_attr + copied, 0, sizeof(local_attr) - copied);
+
+	/*
+	 * The BPF program copies PERF_ATTR_SIZE_VER0 bytes when the tracee
+	 * passes size=0, but leaves the size field as it found it.
+	 */
+	if (!local_attr.size)
 		local_attr.size = PERF_ATTR_SIZE_VER0;
-		attr = &local_attr;
-	}
 
-	return perf_event_attr___scnprintf(attr, bf, size,
+	return perf_event_attr___scnprintf(&local_attr, bf, size,
 					   trace__show_zeros(arg->trace));
 }
 
 size_t syscall_arg__scnprintf_perf_event_attr(char *bf, size_t size, struct syscall_arg *arg)
 {
-	if (arg->augmented.args)
+	if (syscall_arg__augmented_args_valid(arg, PERF_ATTR_SIZE_VER0))
 		return syscall_arg__scnprintf_augmented_perf_event_attr(arg, bf, size);
 
 	return scnprintf(bf, size, "%#lx", arg->val);
diff --git a/tools/perf/trace/beauty/sockaddr.c b/tools/perf/trace/beauty/sockaddr.c
index a17a27ac2a6f..173402400018 100644
--- a/tools/perf/trace/beauty/sockaddr.c
+++ b/tools/perf/trace/beauty/sockaddr.c
@@ -10,21 +10,36 @@
 #include "trace/beauty/generated/sockaddr.c"
 DEFINE_STRARRAY(socket_families, "PF_");
 
-static size_t af_inet__scnprintf(struct sockaddr *sa, char *bf, size_t size)
+/*
+ * The augmented payload is only as long as the addrlen the tracee passed, so
+ * each of these gets that length and prints nothing rather than reading a
+ * struct that isn't fully there.
+ */
+static size_t af_inet__scnprintf(struct sockaddr *sa, size_t sa_size, char *bf, size_t size)
 {
 	struct sockaddr_in *sin = (struct sockaddr_in *)sa;
 	char tmp[16];
+
+	if (sa_size < sizeof(*sin))
+		return 0;
+
 	return scnprintf(bf, size, ", port: %d, addr: %s", ntohs(sin->sin_port),
 			 inet_ntop(sin->sin_family, &sin->sin_addr, tmp, sizeof(tmp)));
 }
 
-static size_t af_inet6__scnprintf(struct sockaddr *sa, char *bf, size_t size)
+static size_t af_inet6__scnprintf(struct sockaddr *sa, size_t sa_size, char *bf, size_t size)
 {
 	struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)sa;
-	u32 flowinfo = ntohl(sin6->sin6_flowinfo);
+	u32 flowinfo;
 	char tmp[512];
-	size_t printed = scnprintf(bf, size, ", port: %d, addr: %s", ntohs(sin6->sin6_port),
-				   inet_ntop(sin6->sin6_family, &sin6->sin6_addr, tmp, sizeof(tmp)));
+	size_t printed;
+
+	if (sa_size < sizeof(*sin6))
+		return 0;
+
+	flowinfo = ntohl(sin6->sin6_flowinfo);
+	printed = scnprintf(bf, size, ", port: %d, addr: %s", ntohs(sin6->sin6_port),
+			    inet_ntop(sin6->sin6_family, &sin6->sin6_addr, tmp, sizeof(tmp)));
 	if (flowinfo != 0)
 		printed += scnprintf(bf + printed, size - printed, ", flowinfo: %lu", flowinfo);
 	if (sin6->sin6_scope_id != 0)
@@ -33,13 +48,28 @@ static size_t af_inet6__scnprintf(struct sockaddr *sa, char *bf, size_t size)
 	return printed;
 }
 
-static size_t af_local__scnprintf(struct sockaddr *sa, char *bf, size_t size)
+static size_t af_local__scnprintf(struct sockaddr *sa, size_t sa_size, char *bf, size_t size)
 {
 	struct sockaddr_un *sun = (struct sockaddr_un *)sa;
-	return scnprintf(bf, size, ", path: %s", sun->sun_path);
+	size_t path_size;
+
+	if (sa_size <= offsetof(struct sockaddr_un, sun_path))
+		return 0;
+
+	/*
+	 * Unlike the other families the path is variable length, and it need
+	 * not be NUL terminated, so print at most what the payload holds. An
+	 * abstract socket's name starts with a NUL, which %.*s stops at, as
+	 * printing it as a plain string used to.
+	 */
+	path_size = sa_size - offsetof(struct sockaddr_un, sun_path);
+	if (path_size > sizeof(sun->sun_path))
+		path_size = sizeof(sun->sun_path);
+
+	return scnprintf(bf, size, ", path: %.*s", (int)path_size, sun->sun_path);
 }
 
-static size_t (*af_scnprintfs[])(struct sockaddr *sa, char *bf, size_t size) = {
+static size_t (*af_scnprintfs[])(struct sockaddr *sa, size_t sa_size, char *bf, size_t size) = {
 	[AF_LOCAL] = af_local__scnprintf,
 	[AF_INET]  = af_inet__scnprintf,
 	[AF_INET6] = af_inet6__scnprintf,
@@ -47,7 +77,9 @@ static size_t (*af_scnprintfs[])(struct sockaddr *sa, char *bf, size_t size) = {
 
 static size_t syscall_arg__scnprintf_augmented_sockaddr(struct syscall_arg *arg, char *bf, size_t size)
 {
-	struct sockaddr *sa = (struct sockaddr *)&arg->augmented.args->value;
+	const struct augmented_arg *augmented_arg = arg->augmented.args;
+	struct sockaddr *sa = (struct sockaddr *)&augmented_arg->value;
+	size_t sa_size = (size_t)augmented_arg->size;
 	char family[32];
 	size_t printed;
 
@@ -55,14 +87,15 @@ static size_t syscall_arg__scnprintf_augmented_sockaddr(struct syscall_arg *arg,
 	printed = scnprintf(bf, size, "{ .family: %s", family);
 
 	if (sa->sa_family < ARRAY_SIZE(af_scnprintfs) && af_scnprintfs[sa->sa_family])
-		printed += af_scnprintfs[sa->sa_family](sa, bf + printed, size - printed);
+		printed += af_scnprintfs[sa->sa_family](sa, sa_size, bf + printed, size - printed);
 
 	return printed + scnprintf(bf + printed, size - printed, " }");
 }
 
 size_t syscall_arg__scnprintf_sockaddr(char *bf, size_t size, struct syscall_arg *arg)
 {
-	if (arg->augmented.args)
+	/* Only the family is read unconditionally, the rest is up to the family printer. */
+	if (syscall_arg__augmented_args_valid(arg, sizeof(sa_family_t)))
 		return syscall_arg__scnprintf_augmented_sockaddr(arg, bf, size);
 
 	return scnprintf(bf, size, "%#lx", arg->val);
diff --git a/tools/perf/trace/beauty/timespec.c b/tools/perf/trace/beauty/timespec.c
index b14ab72a2738..8da0b28be0af 100644
--- a/tools/perf/trace/beauty/timespec.c
+++ b/tools/perf/trace/beauty/timespec.c
@@ -14,7 +14,7 @@ static size_t syscall_arg__scnprintf_augmented_timespec(struct syscall_arg *arg,
 
 size_t syscall_arg__scnprintf_timespec(char *bf, size_t size, struct syscall_arg *arg)
 {
-	if (arg->augmented.args)
+	if (syscall_arg__augmented_args_valid(arg, sizeof(struct timespec)))
 		return syscall_arg__scnprintf_augmented_timespec(arg, bf, size);
 
 	return scnprintf(bf, size, "%#lx", arg->val);
-- 
2.56.0.rc1.315.gc6ed9934b7-goog