Re: [PATCH bpf-next v2 12/17] bpf: Report Execution Context Safety errors

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Integer overflow and out-of-bounds write in `bpf_bprintf_prepare` due to `snprintf` return value mishandling.
--

commit 9da2a659fbd6ab09232d9dfa84f7ad227b09b2a1
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Report Execution Context Safety errors

This commit augments the BPF verifier to report Execution Context Safety
errors. It provides source context, path history, and suggestions for
sleepability and critical-section failures.

While reviewing the BPF codebase, I noticed a critical pre-existing issue in
kernel/bpf/helpers.c within bpf_bprintf_prepare().

> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -818, ... @@ int bpf_bprintf_prepare(const char *fmt, u32 fmt_size, const u64 *raw_args,
>  			u32 num_args, struct bpf_bprintf_data *data)
>  {

[ ... ]

>  			if (fmt[i + 1] == 'B') {
>  				if (tmp_buf)  {
>  					err = snprintf(tmp_buf,
>  						       (tmp_buf_end - tmp_buf),
>  						       "%pB",
>  						       (void *)(long)raw_args[num_spec]);
>  					tmp_buf += (err + 1);

[Severity: Critical]
This is a pre-existing issue, but does this handle the snprintf return value
safely?

The snprintf function returns the number of characters that would have been
written if the buffer was large enough, not the actual number of characters
written. If the output is truncated because the remaining buffer space is
small, adding the return value to tmp_buf could advance the pointer past
tmp_buf_end.

[ ... ]

>  			/* hack: bstr_printf expects IP addresses to be
>  			 * pre-formatted as strings, ironically, the easiest way
>  			 * to do that is to call snprintf.
>  			 */
>  			ip_spec[2] = fmt[i - 1];
>  			ip_spec[3] = fmt[i];
>  			err = snprintf(tmp_buf, tmp_buf_end - tmp_buf,
>  				       ip_spec, &cur_ip);
>  
>  			tmp_buf += err + 1;

[Severity: Critical]
This is a pre-existing issue, but the same snprintf return value behavior
applies here. Could this also advance tmp_buf past tmp_buf_end if the output
is truncated?

[ ... ]

>  		} else if (fmt[i] == 's') {
>  			fmt_ptype = fmt[i];
>  fmt_str:
>  			if (fmt[i + 1] != 0 &&
>  			    !isspace(fmt[i + 1]) &&
>  			    !ispunct(fmt[i + 1])) {
>  				err = -EINVAL;
>  				goto out;
>  			}
>  
>  			if (!tmp_buf)
>  				goto nocopy_fmt;
>  
>  			if (tmp_buf_end == tmp_buf) {
>  				err = -ENOSPC;
>  				goto out;
>  			}
>  
>  			unsafe_ptr = (char *)(long)raw_args[num_spec];
>  			err = bpf_trace_copy_string(tmp_buf, unsafe_ptr,
>  						    fmt_ptype,
>  						    tmp_buf_end - tmp_buf);

[Severity: Critical]
This is a pre-existing issue, but can this bypass the out-of-space check?

If tmp_buf was advanced past tmp_buf_end by a previous truncated snprintf
call, the exact equality check (tmp_buf_end == tmp_buf) will evaluate to
false.

This would cause the size parameter (tmp_buf_end - tmp_buf) passed to
bpf_trace_copy_string to be negative. Since the size parameter is typically
treated as an unsigned value, this could result in a massive out-of-bounds
write. Could we use a greater-than-or-equal check or track the remaining
length more safely?

>  			if (err < 0) {
>  				tmp_buf[0] = '\0';
>  				err = 1;
>  			}
>  
>  			tmp_buf += err;
>  			num_spec++;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260619205934.1312876-1-memxor@gmail.com?part=12