Re: [PATCH v3 1/2] bpf: Fix undefined behavior in interpreter sdiv/smod for INT_MIN

[Yonghong Song <yonghong.song@linux.dev>]

On 3/9/26 11:57 AM, Jenny Guanni Qu wrote:
> The BPF interpreter's signed 32-bit division and modulo handlers use
> the kernel abs() macro on s32 operands. The abs() macro documentation
> (include/linux/math.h) explicitly states the result is undefined when
> the input is the type minimum. When DST contains S32_MIN (0x80000000),
> abs((s32)DST) triggers undefined behavior and returns S32_MIN unchanged
> on arm64/x86. This value is then sign-extended to u64 as
> 0xFFFFFFFF80000000, causing do_div() to compute the wrong result.
>
> The verifier's abstract interpretation (scalar32_min_max_sdiv) computes
> the mathematically correct result for range tracking, creating a
> verifier/interpreter mismatch that can be exploited for out-of-bounds
> map value access.
>
> Introduce __safe_abs32() which handles S32_MIN correctly by casting
> to u32 before negating, avoiding signed overflow entirely. Replace
> all 8 abs((s32)...) call sites in the interpreter's sdiv32/smod32
> handlers.
>
> Fixes: ec0e2da95f72 ("bpf: Support new signed div/mod instructions.")
> Signed-off-by: Jenny Guanni Qu <qguanni@gmail.com>

LGTM with a nit below.

Acked-by: Yonghong Song <yonghong.song@linux.dev>

> ---
>   kernel/bpf/core.c | 23 ++++++++++++++---------
>   1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 3ece2da55625..0bee54db03c1 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -16,7 +16,6 @@
>    * Andi Kleen - Fix a few bad bugs and races.
>    * Kris Katterjohn - Added many additional checks in bpf_check_classic()
>    */
> -

You should not change the above line.

>   #include <uapi/linux/btf.h>
>   #include <crypto/sha1.h>
>   #include <linux/filter.h>
> @@ -1736,6 +1735,12 @@ bool bpf_opcode_in_insntable(u8 code)
>   }
>   
>   #ifndef CONFIG_BPF_JIT_ALWAYS_ON
> +/* Safe absolute value for s32 - abs() is undefined for S32_MIN */

Maybe
    /* Safe absolute value for s32 to prevent undefined behavior for abs(S32_MIN) */
?

> +static inline u32 __safe_abs32(s32 x)
> +{
> +	return x >= 0 ? (u32)x : -(u32)x;
> +}
> +
>   /**
>    *	___bpf_prog_run - run eBPF program on a given context
>    *	@regs: is the array of MAX_BPF_EXT_REG eBPF pseudo-registers
> @@ -1900,8 +1905,8 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn)
>   			DST = do_div(AX, (u32) SRC);
>   			break;
>   		case 1:
> -			AX = abs((s32)DST);
> -			AX = do_div(AX, abs((s32)SRC));
> +			AX = __safe_abs32((s32)DST);
> +			AX = do_div(AX, __safe_abs32((s32)SRC));
>   			if ((s32)DST < 0)
>   				DST = (u32)-AX;
>   			else
> @@ -1928,8 +1933,8 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn)
>   			DST = do_div(AX, (u32) IMM);
>   			break;
>   		case 1:
> -			AX = abs((s32)DST);
> -			AX = do_div(AX, abs((s32)IMM));
> +			AX = __safe_abs32((s32)DST);
> +			AX = do_div(AX, __safe_abs32((s32)IMM));
>   			if ((s32)DST < 0)
>   				DST = (u32)-AX;
>   			else
> @@ -1955,8 +1960,8 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn)
>   			DST = (u32) AX;
>   			break;
>   		case 1:
> -			AX = abs((s32)DST);
> -			do_div(AX, abs((s32)SRC));
> +			AX = __safe_abs32((s32)DST);
> +			do_div(AX, __safe_abs32((s32)SRC));
>   			if (((s32)DST < 0) == ((s32)SRC < 0))
>   				DST = (u32)AX;
>   			else
> @@ -1982,8 +1987,8 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn)
>   			DST = (u32) AX;
>   			break;
>   		case 1:
> -			AX = abs((s32)DST);
> -			do_div(AX, abs((s32)IMM));
> +			AX = __safe_abs32((s32)DST);
> +			do_div(AX, __safe_abs32((s32)IMM));
>   			if (((s32)DST < 0) == ((s32)IMM < 0))
>   				DST = (u32)AX;
>   			else