On 11/19/19 2:00 PM, Alexei Starovoitov wrote:
> 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() ?
tnum_unknown should work since subreg {smin,smax,umin,umax}_value
all in 32-bit range. The mask (-1) should work as upper 32-bit unsigned
value is always 0.
Will make the change and send another revision.
>
>> +
>> /* 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);
>> +}
>
