Re: [PATCH v2 1/3] bpf: Fix truncation bug in coerce_reg_to_size_sx()

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

On 10/14/24 5:11 AM, Dimitar Kanaliev wrote:
> coerce_reg_to_size_sx() updates the register state after a sign-extension
> operation. However, there's a bug in the assignment order of the unsigned
> min/max values, leading to incorrect truncation:
>
>    0: (85) call bpf_get_prandom_u32#7    ; R0_w=scalar()
>    1: (57) r0 &= 1                       ; R0_w=scalar(smin=smin32=0,smax=umax=smax32=umax32=1,var_off=(0x0; 0x1))
>    2: (07) r0 += 254                     ; R0_w=scalar(smin=umin=smin32=umin32=254,smax=umax=smax32=umax32=255,var_off=(0xfe; 0x1))
>    3: (bf) r0 = (s8)r0                   ; R0_w=scalar(smin=smin32=-2,smax=smax32=-1,umin=umin32=0xfffffffe,umax=0xffffffff,var_off=(0xfffffffffffffffe; 0x1))
>
> In the current implementation, the unsigned 32-bit min/max values
> (u32_min_value and u32_max_value) are assigned directly from the 64-bit
> signed min/max values (s64_min and s64_max):
>
>    reg->umin_value = reg->u32_min_value = s64_min;
>    reg->umax_value = reg->u32_max_value = s64_max;
>
> Due to the chain assigmnent, this is equivalent to:
>
>    reg->u32_min_value = s64_min;  // Unintended truncation
>    reg->umin_value = reg->u32_min_value;
>    reg->u32_max_value = s64_max;  // Unintended truncation
>    reg->umax_value = reg->u32_max_value;
>
> Fixes: 1f9a1ea821ff ("bpf: Support new sign-extension load insns")
> Reported-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>
> Reported-by: Zac Ecob <zacecob@protonmail.com>
> Signed-off-by: Dimitar Kanaliev <dimitar.kanaliev@siteground.com>

Thanks for the fix!

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