On Mon, Oct 14, 2024 at 03:11:53PM GMT, 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;
Nit: while I initially suggested the above fragment to Dimitar to use in
commit message, perhaps saying that "reg->u32_min_value = s64_min" is an
unintended truncation is not entirely correct; we do want truncation in
"reg->u32_max_value = (u32)s64_max" to happen, just not
"reg->umax_value = (u32)s64_max". Hopefully the maintainer knows a more
elegant way to put it.
Other than that,
Reviewed-by: Shung-Hsi Yu <shung-hsi.yu@xxxxxxxx>
> Fixes: 1f9a1ea821ff ("bpf: Support new sign-extension load insns")
> Reported-by: Shung-Hsi Yu <shung-hsi.yu@xxxxxxxx>
> Reported-by: Zac Ecob <zacecob@xxxxxxxxxxxxxx>
> Signed-off-by: Dimitar Kanaliev <dimitar.kanaliev@xxxxxxxxxxxxxx>
...
