Re: [PATCH v2 perf-tools-next 5/6] perf trace beauty: Validate payload size in augmented sockaddr beautifier

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Hardcoded address families in the validation logic break extensibility for future formatters.
- [Low] Missing explicit POSIX / Standard C header inclusions for newly introduced types and macros.

Pre-existing issues:
- [High] Missing null-termination enforcement in AF_LOCAL socket paths leads to out-of-bounds reads and abstract socket logic errors.
--

commit 229e56bcf8519abca8347b48fb3e2d0bdc74e111
Author: Aaron Tomlin <atomlin@atomlin.com>

perf trace beauty: Validate payload size in augmented sockaddr beautifier

This commit adds size validation to the augmented sockaddr beautifier to
ensure the captured payload is large enough before dereferencing address
family-specific fields. It verifies the payload size against the augmented
argument bounds and checks for sufficient bytes for AF_INET, AF_INET6, and
AF_LOCAL before formatting.

> diff --git a/tools/perf/trace/beauty/sockaddr.c b/tools/perf/trace/beauty/sockaddr.c
> index a17a27ac2a6ff..00c702ff4465b 100644
> --- a/tools/perf/trace/beauty/sockaddr.c
> +++ b/tools/perf/trace/beauty/sockaddr.c
[ ... ]
> @@ -47,23 +47,48 @@ static size_t (*af_scnprintfs[])(struct sockaddr *sa, char *bf, size_t size) = {
[ ... ]
>  	if (sa->sa_family < ARRAY_SIZE(af_scnprintfs) && af_scnprintfs[sa->sa_family]) {
> +		bool valid = false;

[Severity: Low]
Does this file miss the <stdbool.h> header for the bool type?

The patch introduces the usage of bool in tools/perf/trace/beauty/sockaddr.c,
but the explicit POSIX standard header is missing, which can cause compilation
failures on systems using musl libc.

> +
> +		if (sa->sa_family == AF_INET && augmented_arg->size >= (int)sizeof(struct sockaddr_in))
> +			valid = true;
> +		else if (sa->sa_family == AF_INET6 && augmented_arg->size >= (int)sizeof(struct sockaddr_in6))
> +			valid = true;

[Severity: Medium]
Does this hardcoded validation bypass the extensibility of the af_scnprintfs
dispatch table?

If a new address family formatter (e.g., AF_NETLINK) is added to the
af_scnprintfs dispatch array in the future, it appears it will never be
invoked because the hardcoded validation logic only explicitly recognizes
AF_INET, AF_INET6, and AF_LOCAL.

> +		else if (sa->sa_family == AF_LOCAL &&
> +			 augmented_arg->size > (int)offsetof(struct sockaddr_un, sun_path))

[Severity: Low]
Does this file miss the <stddef.h> header for the offsetof macro?

Similar to bool, the patch introduces offsetof but fails to explicitly include
<stddef.h>, which is required for musl compatibility.

> +			valid = true;

[Severity: High]
This is a pre-existing issue, but does this newly introduced validation fail
to enforce NUL-termination or bound the string length for AF_LOCAL paths?

This explicitly marks the payload as valid without ensuring NUL termination.
Later in syscall_arg__scnprintf_augmented_sockaddr(), the formatter is called
without passing the actual bounds of the captured sockaddr payload:

tools/perf/trace/beauty/sockaddr.c:syscall_arg__scnprintf_augmented_sockaddr() {
    ...
    if (valid)
        printed += af_scnprintfs[sa->sa_family](sa, bf + printed, size - printed);
    ...
}

This leaves af_local__scnprintf() unable to safely bound the read when
formatting sun->sun_path with the "%s" specifier. If the captured payload in
perf.data lacks a NUL terminator, scnprintf() will read past the validated
bounds of the augmented_arg buffer.

> +
> +		if (valid)
> +			printed += af_scnprintfs[sa->sa_family](sa, bf + printed, size - printed);
> +	}
>  
>  	return printed + scnprintf(bf + printed, size - printed, " }");
>  }
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907015140.363076-1-atomlin@atomlin.com?part=5