• John Fastabend's avatar
    bpf: Test_verifier, #70 error message updates for 32-bit right shift · aa131ed4
    John Fastabend authored
    After changes to add update_reg_bounds after ALU ops and adding ALU32
    bounds tracking the error message is changed in the 32-bit right shift
    tests.
    
    Test "#70/u bounds check after 32-bit right shift with 64-bit input FAIL"
    now fails with,
    
    Unexpected error message!
    	EXP: R0 invalid mem access
    	RES: func#0 @0
    
    7: (b7) r1 = 2
    8: R0_w=map_value(id=0,off=0,ks=8,vs=8,imm=0) R1_w=invP2 R10=fp0 fp-8_w=mmmmmmmm
    8: (67) r1 <<= 31
    9: R0_w=map_value(id=0,off=0,ks=8,vs=8,imm=0) R1_w=invP4294967296 R10=fp0 fp-8_w=mmmmmmmm
    9: (74) w1 >>= 31
    10: R0_w=map_value(id=0,off=0,ks=8,vs=8,imm=0) R1_w=invP0 R10=fp0 fp-8_w=mmmmmmmm
    10: (14) w1 -= 2
    11: R0_w=map_value(id=0,off=0,ks=8,vs=8,imm=0) R1_w=invP4294967294 R10=fp0 fp-8_w=mmmmmmmm
    11: (0f) r0 += r1
    math between map_value pointer and 4294967294 is not allowed
    
    And test "#70/p bounds check after 32-bit right shift with 64-bit input
    FAIL" now fails with,
    
    Unexpected error message!
    	EXP: R0 invalid mem access
    	RES: func#0 @0
    
    7: (b7) r1 = 2
    8: R0_w=map_value(id=0,off=0,ks=8,vs=8,imm=0) R1_w=inv2 R10=fp0 fp-8_w=mmmmmmmm
    8: (67) r1 <<= 31
    9: R0_w=map_value(id=0,off=0,ks=8,vs=8,imm=0) R1_w=inv4294967296 R10=fp0 fp-8_w=mmmmmmmm
    9: (74) w1 >>= 31
    10: R0_w=map_value(id=0,off=0,ks=8,vs=8,imm=0) R1_w=inv0 R10=fp0 fp-8_w=mmmmmmmm
    10: (14) w1 -= 2
    11: R0_w=map_value(id=0,off=0,ks=8,vs=8,imm=0) R1_w=inv4294967294 R10=fp0 fp-8_w=mmmmmmmm
    11: (0f) r0 += r1
    last_idx 11 first_idx 0
    regs=2 stack=0 before 10: (14) w1 -= 2
    regs=2 stack=0 before 9: (74) w1 >>= 31
    regs=2 stack=0 before 8: (67) r1 <<= 31
    regs=2 stack=0 before 7: (b7) r1 = 2
    math between map_value pointer and 4294967294 is not allowed
    
    Before this series we did not trip the "math between map_value pointer..."
    error because check_reg_sane_offset is never called in
    adjust_ptr_min_max_vals(). Instead we have a register state that looks
    like this at line 11*,
    
    11: R0_w=map_value(id=0,off=0,ks=8,vs=8,
                       smin_value=0,smax_value=0,
                       umin_value=0,umax_value=0,
                       var_off=(0x0; 0x0))
        R1_w=invP(id=0,
                  smin_value=0,smax_value=4294967295,
                  umin_value=0,umax_value=4294967295,
                  var_off=(0xfffffffe; 0x0))
        R10=fp(id=0,off=0,
               smin_value=0,smax_value=0,
               umin_value=0,umax_value=0,
               var_off=(0x0; 0x0)) fp-8_w=mmmmmmmm
    11: (0f) r0 += r1
    
    In R1 'smin_val != smax_val' yet we have a tnum_const as seen
    by 'var_off(0xfffffffe; 0x0))' with a 0x0 mask. So we hit this check
    in adjust_ptr_min_max_vals()
    
     if ((known && (smin_val != smax_val || umin_val != umax_val)) ||
          smin_val > smax_val || umin_val > umax_val) {
           /* Taint dst register if offset had invalid bounds derived from
            * e.g. dead branches.
            */
           __mark_reg_unknown(env, dst_reg);
           return 0;
     }
    
    So we don't throw an error here and instead only throw an error
    later in the verification when the memory access is made.
    
    The root cause in verifier without alu32 bounds tracking is having
    'umin_value = 0' and 'umax_value = U64_MAX' from BPF_SUB which we set
    when 'umin_value < umax_val' here,
    
     if (dst_reg->umin_value < umax_val) {
        /* Overflow possible, we know nothing */
        dst_reg->umin_value = 0;
        dst_reg->umax_value = U64_MAX;
     } else { ...}
    
    Later in adjust_calar_min_max_vals we previously did a
    coerce_reg_to_size() which will clamp the U64_MAX to U32_MAX by
    truncating to 32bits. But either way without a call to update_reg_bounds
    the less precise bounds tracking will fall out of the alu op
    verification.
    
    After latest changes we now exit adjust_scalar_min_max_vals with the
    more precise umin value, due to zero extension propogating bounds from
    alu32 bounds into alu64 bounds and then calling update_reg_bounds.
    This then causes the verifier to trigger an earlier error and we get
    the error in the output above.
    
    This patch updates tests to reflect new error message.
    
    * I have a local patch to print entire verifier state regardless if we
     believe it is a constant so we can get a full picture of the state.
     Usually if tnum_is_const() then bounds are also smin=smax, etc. but
     this is not always true and is a bit subtle. Being able to see these
     states helps understand dataflow imo. Let me know if we want something
     similar upstream.
    Signed-off-by: default avatarJohn Fastabend <john.fastabend@gmail.com>
    Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
    Link: https://lore.kernel.org/bpf/158507161475.15666.3061518385241144063.stgit@john-Precision-5820-Tower
    aa131ed4
bounds.c 16.2 KB