lkml.org 
[lkml]   [2017]   [Jun]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3 net-next 02/12] bpf/verifier: rework value tracking
    On 06/27/2017 02:56 PM, Edward Cree wrote:
    > Tracks value alignment by means of tracking known & unknown bits.
    > Tightens some min/max value checks and fixes a couple of bugs therein.

    You mean the one in relation to patch 1/12? Would be good to elaborate
    here since otherwise this gets forgotten few weeks later.

    Could you also document all the changes that verifier will then start
    allowing for after the patch?

    > If pointer leaks are allowed, and adjust_ptr_min_max_vals returns -EACCES,
    > treat the pointer as an unknown scalar and try again, because we might be
    > able to conclude something about the result (e.g. pointer & 0x40 is either
    > 0 or 0x40).
    >
    > Signed-off-by: Edward Cree <ecree@solarflare.com>
    [...]
    > /* check whether memory at (regno + off) is accessible for t = (read | write)
    > @@ -899,52 +965,79 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
    > struct bpf_reg_state *reg = &state->regs[regno];
    > int size, err = 0;
    >
    > - if (reg->type == PTR_TO_STACK)
    > - off += reg->imm;
    > -
    > size = bpf_size_to_bytes(bpf_size);
    > if (size < 0)
    > return size;
    >
    [...]
    > - if (reg->type == PTR_TO_MAP_VALUE ||
    > - reg->type == PTR_TO_MAP_VALUE_ADJ) {
    > + /* for access checks, reg->off is just part of off */
    > + off += reg->off;

    Could you elaborate on why removing the reg->type == PTR_TO_STACK?
    Also in context of below PTR_TO_CTX.

    [...]
    > } else if (reg->type == PTR_TO_CTX) {
    > - enum bpf_reg_type reg_type = UNKNOWN_VALUE;
    > + enum bpf_reg_type reg_type = SCALAR_VALUE;
    >
    > if (t == BPF_WRITE && value_regno >= 0 &&
    > is_pointer_value(env, value_regno)) {
    > verbose("R%d leaks addr into ctx\n", value_regno);
    > return -EACCES;
    > }
    > + /* ctx accesses must be at a fixed offset, so that we can
    > + * determine what type of data were returned.
    > + */
    > + if (!tnum_is_const(reg->var_off)) {
    > + char tn_buf[48];
    > +
    > + tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off);
    > + verbose("variable ctx access var_off=%s off=%d size=%d",
    > + tn_buf, off, size);
    > + return -EACCES;
    > + }
    > + off += reg->var_off.value;

    ... f.e. in PTR_TO_CTX case the only access that is currently
    allowed is LDX/STX with fixed offset from insn->off, which is
    passed as off param to check_mem_access(). Can you elaborate on
    off += reg->var_off.value? Meaning we make this more dynamic
    as long as access is known const?

    > err = check_ctx_access(env, insn_idx, off, size, t, &reg_type);
    > if (!err && t == BPF_READ && value_regno >= 0) {
    > - mark_reg_unknown_value_and_range(state->regs,
    > - value_regno);
    > - /* note that reg.[id|off|range] == 0 */
    > + /* ctx access returns either a scalar, or a
    > + * PTR_TO_PACKET[_END]. In the latter case, we know
    > + * the offset is zero.
    > + */
    > + if (reg_type == SCALAR_VALUE)
    > + mark_reg_unknown(state->regs, value_regno);
    > + else
    > + mark_reg_known_zero(state->regs, value_regno);
    > + state->regs[value_regno].id = 0;
    > + state->regs[value_regno].off = 0;
    > + state->regs[value_regno].range = 0;
    > state->regs[value_regno].type = reg_type;
    > - state->regs[value_regno].aux_off = 0;
    > - state->regs[value_regno].aux_off_align = 0;
    > }
    >
    > - } else if (reg->type == FRAME_PTR || reg->type == PTR_TO_STACK) {
    > + } else if (reg->type == PTR_TO_STACK) {
    [...]

    \
     
     \ /
      Last update: 2017-06-28 17:16    [W:6.869 / U:0.180 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site