On Tue, Nov 19, 2019 at 11:57:12AM -0800, Yonghong Song wrote:
> #define TNUM(_v, _m) (struct tnum){.value = _v, .mask = _m}
> -/* A completely unknown value */
> +/* completely unknown 32-bit and 64-bit values */
> +const struct tnum tnum_unknown32 = { .value = 0, .mask = 0xffffffffULL };
> const struct tnum tnum_unknown = { .value = 0, .mask = -1 };
>
> struct tnum tnum_const(u64 value)
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index a344b08aef77..945827351758 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1024,6 +1024,15 @@ static void __mark_reg_unbounded(struct bpf_reg_state *reg)
> reg->umax_value = U64_MAX;
> }
>
> +/* Reset the min/max bounds of a sub register */
> +static void __mark_subreg_unbounded(struct bpf_reg_state *subreg)
> +{
> + subreg->smin_value = S32_MIN;
> + subreg->smax_value = S32_MAX;
> + subreg->umin_value = 0;
> + subreg->umax_value = U32_MAX;
> +}
when int32 is returned the above feels correct, but I think it conflicts with
definition of tnum_unknown32, since it says that upper 32-bit should be zero.
The typical verifier action after processing alu32 insn:
if (BPF_CLASS(insn->code) != BPF_ALU64) {
/* 32-bit ALU ops are (32,32)->32 */
coerce_reg_to_size(dst_reg, 4);
}
__reg_deduce_bounds(dst_reg);
__reg_bound_offset(dst_reg);
And that is correct behavior for alu32, but here the helper is returning
'int', so if the verifier says subreg->smin_value = S32_MIN;
it means that upper bits will be non-zero.
The helper can return (u64)-1 with all 64-bits being set to 1.
If next insn after w0 = call helper; is w0 += imm;
the verifier will do above coerce+deduce logic and clear upper bits.
That's correct, but without extra alu32 operation on w0 the state
of r0 is technically correct, but doesn't match r0->var_reg
which is tnum_unknown32.
I wonder whether it should be tnum_unknown instead with above
__mark_subreg_unbounded() ?
>+
> /* Mark a register as having a completely unknown (scalar) value. */
> static void __mark_reg_unknown(struct bpf_reg_state *reg)
> {
> @@ -1038,6 +1047,20 @@ static void __mark_reg_unknown(struct bpf_reg_state *reg)
> __mark_reg_unbounded(reg);
> }
>
> +/* Mark a sub register as having a completely unknown (scalar) value. */
> +static void __mark_subreg_unknown(struct bpf_reg_state *subreg)
> +{
> + /*
> + * Clear type, id, off, and union(map_ptr, range) and
> + * padding between 'type' and union
> + */
> + memset(subreg, 0, offsetof(struct bpf_reg_state, var_off));
> + subreg->type = SCALAR_VALUE;
> + subreg->var_off = tnum_unknown32;
> + subreg->frameno = 0;
> + __mark_subreg_unbounded(subreg);
> +}
