Re: [PATCH v6 05/26] perf trace: Bound the fixed size augmented argument beautifiers

[Aaron Tomlin]

On Mon, Sep 28, 2026 at 11:25:44AM -0700, Ian Rogers wrote:
> The timespec, sockaddr and perf_event_attr beautifiers read a fixed size
> type from the payload without checking its size. The sockaddr payload is
> addrlen bytes and the perf_event_attr payload is attr.size bytes, both
> chosen by the tracee.
> 
> Check the payload with syscall_arg__augmented_args_valid(). Pass its size
> to the address family printers, which also bounds the AF_LOCAL path that
> needn't be NUL terminated. Like the kernel, accept an AF_INET6 address
> without sin6_scope_id, RFC 2133's 24 bytes. Copy the perf_event_attr into
> a zero padded local, as perf_event_attr___scnprintf() reads a whole
> struct.
> 
> Assisted-by: Antigravity:gemini-3.1-pro
> Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> ---
>  tools/perf/trace/beauty/perf_event_open.c | 31 ++++++--------
>  tools/perf/trace/beauty/sockaddr.c        | 49 +++++++++++++++++------
>  tools/perf/trace/beauty/timespec.c        |  2 +-
>  3 files changed, 51 insertions(+), 31 deletions(-)
> 
> diff --git a/tools/perf/trace/beauty/perf_event_open.c b/tools/perf/trace/beauty/perf_event_open.c
> index 6315b46bcdf0..bfdf322c6ede 100644
> --- a/tools/perf/trace/beauty/perf_event_open.c
> +++ b/tools/perf/trace/beauty/perf_event_open.c
> @@ -81,33 +81,28 @@ 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.
> -	 */
> -	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);
> +	/* Zero pad, as the tracee's attr may be smaller than perf's. */
> +	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 for size 0. */
> +	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..b4b188879598 100644
> --- a/tools/perf/trace/beauty/sockaddr.c
> +++ b/tools/perf/trace/beauty/sockaddr.c
> @@ -2,6 +2,7 @@
>  // Copyright (C) 2018, Red Hat Inc, Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
>  
>  #include "trace/beauty/beauty.h"
> +#include <stddef.h>
>  #include <sys/socket.h>
>  #include <sys/types.h>
>  #include <sys/un.h>
> @@ -10,36 +11,57 @@
>  #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)
> +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;
> +
> +	/* RFC 2133's version, which the kernel accepts, lacks sin6_scope_id. */
> +	if (sa_size < offsetof(struct sockaddr_in6, sin6_scope_id))
> +		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)
> +	if (sa_size >= sizeof(*sin6) && sin6->sin6_scope_id != 0)
>  		printed += scnprintf(bf + printed, size - printed, ", scope_id: %lu", sin6->sin6_scope_id);
>  
>  	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;
> +
> +	/* The path needn't be NUL terminated. */
> +	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 +69,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 +79,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)
> +	/* The family printers check the rest of the payload. */
> +	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
> 

Reviewed-by: Aaron Tomlin <atomlin@xxxxxxxxxxx>
Tested-by: Aaron Tomlin <atomlin@xxxxxxxxxxx>

-- 
Aaron Tomlin