Re: [PATCH] libpf: Fix for the compiler sanity checking

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

Pre-existing issues:
- [Medium] Latent undefined behavior due to signed left-shift overflow in the `BTF_INFO_ENC()` macro.
- [Medium] Latent undefined behavior due to signed left-shift when setting retprobe bit in `attr.config`.
--

--- Patch [1]: [PATCH] libpf: Fix for the compiler sanity checking ---
commit 6bb550337600fb1ee129a0b5ed1c50e66620bbc5
Author: Tw <[email protected]>

libpf: Fix for the compiler sanity checking

This patch fixes an undefined behavior issue during libbpf compilation.
It resolves a ubsan warning about left shifting 1 by 31 places in an int
by casting the integer to a u32 before performing the shift.

> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index 4c46d34fc055d..222d2f1c0ed3a 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -259,7 +259,7 @@ static inline enum btf_func_linkage btf_func_linkage(const struct btf_type *t)
>  
>  static inline __u32 btf_type_info(int kind, int vlen, int kflag)
>  {
> -	return (kflag << 31) | (kind << 24) | vlen;
> +	return ((__u32)kflag << 31) | (kind << 24) | vlen;
>  }

[Severity: Medium]
This isn't a bug introduced by this patch, but since you are fixing the
shift in btf_type_info(), should we also apply this fix to the BTF_INFO_ENC
macro in this same file?

tools/lib/bpf/libbpf_internal.h:
#define BTF_INFO_ENC(kind, kind_flag, vlen) \
	((!!(kind_flag) << 31) | ((kind) << 24) | ((vlen) & BTF_MAX_VLEN))

The !! operator results in a signed int. If a caller provides a non-zero
kind_flag, shifting it left by 31 places into the sign bit will also result
in undefined behavior. Would it make sense to cast to __u32 here as well?

[Severity: Medium]
This is another pre-existing issue, but I noticed a similar left-shift
that could cause undefined behavior when setting the retprobe bit:

tools/lib/bpf/libbpf.c:perf_event_open_probe()
	if (retprobe) {
		int bit = uprobe ? determine_uprobe_retprobe_bit()
				 : determine_kprobe_retprobe_bit();

		if (bit < 0) {
[ ... ]
			return bit;
		}
		attr.config |= 1 << bit;
	}

Since bit is read from sysfs, if a future kernel update returns a bit index
of 31 or higher, would 1 << bit overflow and incorrectly configure
attr.config? Should this use 1ULL << bit to safely set the 64-bit config
field?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1