linux-kernel - Re: [PATCH v1] perf trace: Fix BPF loading failure (-E2BIG)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z2Nsr2ar6i-1LqKT@google.com>
Date: Wed, 18 Dec 2024 16:45:35 -0800
From: Namhyung Kim <namhyung@...nel.org>
To: Howard Chu <howardchu95@...il.com>
Cc: acme@...nel.org, mark.rutland@....com,
	alexander.shishkin@...ux.intel.com, jolsa@...nel.org,
	irogers@...gle.com, adrian.hunter@...el.com,
	kan.liang@...ux.intel.com, linux-perf-users@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] perf trace: Fix BPF loading failure (-E2BIG)

Hello,

On Thu, Dec 12, 2024 at 06:30:47PM -0800, Howard Chu wrote:
> As reported by Namhyung Kim and acknowledged by Qiao Zhao (link:
> https://lore.kernel.org/linux-perf-users/20241206001436.1947528-1-namhyung@kernel.org/),
> on certain machines, perf trace failed to load the BPF program into the
> kernel. The verifier runs perf trace's BPF program for up to 1 million
> instructions, returning an E2BIG error, whereas the perf trace BPF
> program should be much less complex than that. This patch aims to fix
> the issue described above.
> 
> The E2BIG problem from clang-15 to clang-16 is cause by this line:
>  } else if (size < 0 && size >= -6) { /* buffer */
> 
> Specifically this check: size < 0. seems like clang generates a cool
> optimization to this sign check that breaks things.
> 
> Making 'size' s64, and use
>  } else if ((int)size < 0 && size >= -6) { /* buffer */
> 
> Solves the problem. This is some Hogwarts magic.
> 
> And the unbounded access of clang-12 and clang-14 (clang-13 works this
> time) is fixed by making variable 'aug_size' s64.
> 
> As for this:
> -if (aug_size > TRACE_AUG_MAX_BUF)
> -	aug_size = TRACE_AUG_MAX_BUF;
> +aug_size = args->args[index] > TRACE_AUG_MAX_BUF ? TRACE_AUG_MAX_BUF : args->args[index];
> 
> This makes the BPF skel generated by clang-18 work. Yes, new clangs
> introduce problems too.
> 
> Sorry, I only know that it works, but I don't know how it works. I'm not
> an expert in the BPF verifier. I really hope this is not a kernel
> version issue, as that would make the test case (kernel_nr) *
> (clang_nr), a true horror story. I will test it on more kernel versions
> in the future.
> 
> Fixes: 395d38419f18: ("perf trace augmented_raw_syscalls: Add more check s to pass the verifier")
> Reported-by: Namhyung Kim <namhyung@...nel.org>
> Signed-off-by: Howard Chu <howardchu95@...il.com>

Thanks for the fix!

Tested-by: Namhyung Kim <namhyung@...nel.org>

> ---
>  tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> index 4a62ed593e84..e4352881e3fa 100644
> --- a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> +++ b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> @@ -431,9 +431,9 @@ static bool pid_filter__has(struct pids_filtered *pids, pid_t pid)
>  static int augment_sys_enter(void *ctx, struct syscall_enter_args *args)
>  {
>  	bool augmented, do_output = false;
> -	int zero = 0, size, aug_size, index,
> -	    value_size = sizeof(struct augmented_arg) - offsetof(struct augmented_arg, value);
> +	int zero = 0, index, value_size = sizeof(struct augmented_arg) - offsetof(struct augmented_arg, value);
>  	u64 output = 0; /* has to be u64, otherwise it won't pass the verifier */
> +	s64 aug_size, size;
>  	unsigned int nr, *beauty_map;
>  	struct beauty_payload_enter *payload;
>  	void *arg, *payload_offset;
> @@ -484,14 +484,11 @@ static int augment_sys_enter(void *ctx, struct syscall_enter_args *args)
>  		} else if (size > 0 && size <= value_size) { /* struct */
>  			if (!bpf_probe_read_user(((struct augmented_arg *)payload_offset)->value, size, arg))
>  				augmented = true;
> -		} else if (size < 0 && size >= -6) { /* buffer */
> +		} else if ((int)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 = args->args[index];
> -
> -			if (aug_size > TRACE_AUG_MAX_BUF)
> -				aug_size = TRACE_AUG_MAX_BUF;
> +			aug_size = args->args[index] > TRACE_AUG_MAX_BUF ? TRACE_AUG_MAX_BUF : args->args[index];
>  
>  			if (aug_size > 0) {
>  				if (!bpf_probe_read_user(((struct augmented_arg *)payload_offset)->value, aug_size, arg))
> -- 
> 2.43.0
>