[bug report] bpf: Make 32->64 bounds propagation slightly more robust

[bug report] bpf: Make 32->64 bounds propagation slightly more robust

[Dan Carpenter]

Hello Daniel Borkmann,

The patch e572ff80f05c: "bpf: Make 32->64 bounds propagation slightly
more robust" from Dec 15, 2021, leads to the following Smatch static
checker warning:

	kernel/bpf/verifier.c:1412 __reg32_bound_s64()
	warn: always true condition '(a <= (((~0) >> 1))) => (s32min-s32max <= s32max)'

kernel/bpf/verifier.c
  1410        static bool __reg32_bound_s64(s32 a)
  1411        {
  1412                return a >= 0 && a <= S32_MAX;

Obviously an s32 is going to be <= S32_MAX

  1413        }
  1414
  1415        static void __reg_assign_32_into_64(struct bpf_reg_state *reg)
  1416        {
  1417                reg->umin_value = reg->u32_min_value;
  1418                reg->umax_value = reg->u32_max_value;
  1419
  1420                /* Attempt to pull 32-bit signed bounds into 64-bit bounds but must
  1421                 * be positive otherwise set to worse case bounds and refine later
  1422                 * from tnum.
  1423                 */
  1424                if (__reg32_bound_s64(reg->s32_min_value) &&
  1425                    __reg32_bound_s64(reg->s32_max_value)) {
  1426                        reg->smin_value = reg->s32_min_value;
  1427                        reg->smax_value = reg->s32_max_value;
  1428                } else {
  1429                        reg->smin_value = 0;
  1430                        reg->smax_value = U32_MAX;

Should this be S32_MAX instead of U32_MAX?

  1431                }
  1432        }

regards,
dan carpenter

Re: [bug report] bpf: Make 32->64 bounds propagation slightly more robust

[Daniel Borkmann]

On 1/11/22 9:20 AM, Dan Carpenter wrote:
> Hello Daniel Borkmann,

[+John]

> The patch e572ff80f05c: "bpf: Make 32->64 bounds propagation slightly
> more robust" from Dec 15, 2021, leads to the following Smatch static
> checker warning:
> 
> 	kernel/bpf/verifier.c:1412 __reg32_bound_s64()
> 	warn: always true condition '(a <= (((~0) >> 1))) => (s32min-s32max <= s32max)'
> 
> kernel/bpf/verifier.c
>    1410        static bool __reg32_bound_s64(s32 a)
>    1411        {
>    1412                return a >= 0 && a <= S32_MAX;
> 
> Obviously an s32 is going to be <= S32_MAX

It's aligned with similar helpers such as __reg64_bound_u32() / __reg64_bound_s32() and
when discussing we went for leaving this explicitly documented in here (aside being true).

>    1413        }
>    1414
>    1415        static void __reg_assign_32_into_64(struct bpf_reg_state *reg)
>    1416        {
>    1417                reg->umin_value = reg->u32_min_value;
>    1418                reg->umax_value = reg->u32_max_value;
>    1419
>    1420                /* Attempt to pull 32-bit signed bounds into 64-bit bounds but must
>    1421                 * be positive otherwise set to worse case bounds and refine later
>    1422                 * from tnum.
>    1423                 */
>    1424                if (__reg32_bound_s64(reg->s32_min_value) &&
>    1425                    __reg32_bound_s64(reg->s32_max_value)) {
>    1426                        reg->smin_value = reg->s32_min_value;
>    1427                        reg->smax_value = reg->s32_max_value;
>    1428                } else {
>    1429                        reg->smin_value = 0;
>    1430                        reg->smax_value = U32_MAX;
> 
> Should this be S32_MAX instead of U32_MAX?

U32_MAX is correct here.

>    1431                }
>    1432        }
> 
> regards,
> dan carpenter
>