Re: [PATCH] perf trace: Refactor augmented_raw_syscalls using bpf_loop

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Removal of the `aug_size < 0` check causes negative BPF helper error codes to corrupt the perf event payload and abort tracing.
- [Medium] Truncation of 64-bit syscall buffer lengths to a 32-bit signed integer silently drops buffer augmentations or bypasses bounds checks.
--

commit 9622318b331a56255744b4baafdc95c2083ba210
Author: Viktor Malik <vmalik@redhat.com>
perf trace: Refactor augmented_raw_syscalls using bpf_loop

This patch refactors the loop body in augment_raw_syscalls into a
callback function to be used with the bpf_loop helper. This helps the
BPF verifier prove termination and avoids -E2BIG load failures.

> diff --git a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> --- a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> +++ b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
[ ... ]
> +static long process_arg_cb(u64 i, void *ctx)
> +{
[ ... ]
> +	struct augmented_arg *augmented_arg;
> +	struct args_loop_ctx *loop_ctx;
> +	int aug_size, size, index;
> +	bool augmented;
> +	void *arg;
[ ... ]
> +	if (size == 1) { /* string */
> +		aug_size = bpf_probe_read_user_str(augmented_arg->value, loop_ctx->value_size, arg);

[Severity: Medium]
Since bpf_probe_read_user_str() can return a negative error code like
-EFAULT, does the removal of the previous clamping check cause an issue
here?

If a negative error code is assigned to aug_size, it might result in a
negative size being added to offsetof() later, which could abort the loop
prematurely or misalign payload offsets:

    int written = offsetof(struct augmented_arg, value) + aug_size;

    if (written < 0 || written > sizeof(struct augmented_arg))
        return 1; /* break */

> +		augmented = true;
> +	} else if (size > 0 && size <= loop_ctx->value_size) { /* struct */
> +		if (!bpf_probe_read_user(augmented_arg->value, size, arg))
> +			augmented = true;
> +	} else if (size < 0 && size >= -6) { /* buffer */
> +		index = -(size + 1);
> +		barrier_var(index); // Prevent clang (noticed with v18) from removing the &= 7 trick.
> +		index &= 7;	    // Satisfy the bounds checking with the verifier in some kernels.
> +		aug_size = loop_ctx->args->args[index];

[Severity: Medium]
Does assigning the 64-bit user-provided buffer length to a 32-bit signed
integer (aug_size) cause truncation before bounds checking is performed?

If a length over 2GB is provided, it might wrap to a negative number and
fail the subsequent aug_size > 0 check. Values over 4GB could wrap to small
positive numbers, bypass the max buffer size clamp, and result in capturing
fewer bytes than expected.

> +
> +		if (aug_size > TRACE_AUG_MAX_BUF)
> +			aug_size = TRACE_AUG_MAX_BUF;
> +
> +		if (aug_size > 0) {
> +			if (!bpf_probe_read_user(augmented_arg->value, aug_size, arg))
> +				augmented = true;
> +		}
> +	}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623112533.1151502-1-vmalik@redhat.com?part=1