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

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

Jenny Guanni Qu <qguanni@gmail.com> writes:

> 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 and 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>
> ---
>  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..663b9af6d19b 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()
>   */
> -
>  #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 */
> +static inline u32 __safe_abs32(s32 x)
> +{
> +	return x >= 0 ? (u32)x : x == S32_MIN ? (u32)S32_MIN : (u32)-x;
can we write it as x >= 0 ? (u32)x : -(u32)x; ?
By casting to u32 first, the negation operates in unsigned arithmetic
where overflow does not happen — it wraps.
Please correct me if I'm wrong.
> +}
> +
>  /**
>   *	___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
> -- 
> 2.34.1