Message ID | 20190301070312.374711-1-kafai@fb.com |
---|---|
State | Superseded |
Delegated to: | BPF Maintainers |
Headers | show |
Series | Fix bpf_tcp_sock and bpf_sk_fullsock issue related to bpf_sk_release | expand |
On Fri, 1 Mar 2019 at 07:03, Martin KaFai Lau <kafai@fb.com> wrote: > > Lorenz Bauer [thanks!] reported that a ptr returned by bpf_tcp_sock(sk) > can still be accessed after bpf_sk_release(sk). > Both bpf_tcp_sock() and bpf_sk_fullsock() have the same issue. > This patch addresses them together. > > A simple reproducer looks like this: > > sk = bpf_sk_lookup_tcp(); > /* if (!sk) ... */ > tp = bpf_tcp_sock(sk); > /* if (!tp) ... */ > bpf_sk_release(sk); > snd_cwnd = tp->snd_cwnd; /* oops! The verifier does not complain. */ > > The problem is the verifier did not scrub the register's states of > the tcp_sock ptr (tp) after bpf_sk_release(sk). > > [ Note that when calling bpf_tcp_sock(sk), the sk is not always > refcount-acquired. e.g. bpf_tcp_sock(skb->sk). The verifier works > fine for this case. ] > > Currently, the verifier does not track if a helper's return ptr (in REG_0) > is "carry"-ing one of its argument's refcount status. To carry this info, > the reg1->id needs to be stored in reg0. The reg0->id has already > been used for NULL checking purpose. Hence, a new "refcount_id" > is needed in "struct bpf_reg_state". > > With refcount_id, when bpf_sk_release(sk) is called, the verifier can scrub > all reg states which has a refcount_id match. It is done with the changes > in release_reg_references(). > > When acquiring and releasing a refcount, the reg->id is still used. > Hence, we cannot do "bpf_sk_release(tp)" in the above reproducer > example. > > Misc change notes: > - With the new refcount_id, reg_is_refcounted() test can now be > done with "reg->refcount_id && reg->id == reg->refcount_id" instead of > testing the ptr type. > > The type_is_refcounted() and type_is_refcounted_or_null() > are no longer needed, so removed. > > - An anonymous struct is added to bpf_call_arg_meta to store > the reg->id and reg->refcount_id of the arg. Otherwise, > they will be unavailable after check_helper_call() > has cleared all CALLER_SAVED_REGS. > > - The check_func_arg() can only allow one refcount-ed arg. It is > guaranteed by check_refcount_ok() which ensures at most one arg can be > refcount-ed. Hence, it is a verifier internal error if >1 refcount arg > found in check_func_arg(). > > - The check_func_arg() also complains if a "is_acquire_function(func_id)" > helper is having a refcount-ed arg. No func_id is doing this now > and should have been rejected earlier, so it is treated as > verifier internal error also. > > - In check_func_arg(), the "!reg->id" check is removed under > the ARG_PTR_TO_SOCKET case. It is because a PTR_TO_SOCKET > can be obtained from bpf_sk_fullsock() and it does not > take a refcount. The verifier will still complain during > release_reference() but it does not treat it as a > verifier internal error anymore. > > - In release_reference(), release_reference_state() is called > first to ensure a match on "reg->id" can be found before > scrubbing the reg states with release_reg_references(). > > Fixes: 655a51e536c0 ("bpf: Add struct bpf_tcp_sock and BPF_FUNC_tcp_sock") > Cc: Lorenz Bauer <lmb@cloudflare.com> > Reported-by: Lorenz Bauer <lmb@cloudflare.com> > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > --- > include/linux/bpf_verifier.h | 9 ++++ > kernel/bpf/verifier.c | 89 ++++++++++++++++++++++-------------- > 2 files changed, 64 insertions(+), 34 deletions(-) > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > index 69f7a3449eda..b7698d0534cb 100644 > --- a/include/linux/bpf_verifier.h > +++ b/include/linux/bpf_verifier.h > @@ -66,6 +66,15 @@ struct bpf_reg_state { > * same reference to the socket, to determine proper reference freeing. > */ > u32 id; > + /* For PTR_TO_SOCKET and PTR_TO_TCP_SOCK, this ptr may not actually > + * hold a refcount of a socket but instead it is a ptr > + * returned from a helper which is based on its refcount-ed > + * ptr argument (e.g. bpf_tcp_sock()). > + * "refcount_id" stores which refcount-ed argument it > + * originally derived from. When this original argument's > + * refcount is released, this ptr will also be invalidated. > + */ > + u32 refcount_id; > /* For scalar types (SCALAR_VALUE), this represents our knowledge of > * the actual value. > * For pointer types, this represents the variable part of the offset > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 1b9496c41383..682b7a6f0d2a 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -212,7 +212,10 @@ struct bpf_call_arg_meta { > int access_size; > s64 msize_smax_value; > u64 msize_umax_value; > - int ptr_id; > + struct { > + u32 id; > + u32 refcount_id; > + } refcount_reg; > int func_id; > }; > > @@ -346,19 +349,9 @@ static bool reg_type_may_be_null(enum bpf_reg_type type) > type == PTR_TO_TCP_SOCK_OR_NULL; > } > > -static bool type_is_refcounted(enum bpf_reg_type type) > -{ > - return type == PTR_TO_SOCKET; > -} > - > -static bool type_is_refcounted_or_null(enum bpf_reg_type type) > -{ > - return type == PTR_TO_SOCKET || type == PTR_TO_SOCKET_OR_NULL; > -} > - > static bool reg_is_refcounted(const struct bpf_reg_state *reg) > { > - return type_is_refcounted(reg->type); > + return reg->refcount_id && reg->id == reg->refcount_id; > } > > static bool reg_may_point_to_spin_lock(const struct bpf_reg_state *reg) > @@ -367,14 +360,10 @@ static bool reg_may_point_to_spin_lock(const struct bpf_reg_state *reg) > map_value_has_spin_lock(reg->map_ptr); > } > > -static bool reg_is_refcounted_or_null(const struct bpf_reg_state *reg) > -{ > - return type_is_refcounted_or_null(reg->type); > -} > - > static bool arg_type_is_refcounted(enum bpf_arg_type type) > { > - return type == ARG_PTR_TO_SOCKET; > + return type == ARG_PTR_TO_SOCKET || > + type == ARG_PTR_TO_SOCK_COMMON; > } > > /* Determine whether the function releases some resources allocated by another > @@ -392,6 +381,12 @@ static bool is_acquire_function(enum bpf_func_id func_id) > func_id == BPF_FUNC_sk_lookup_udp; > } > > +static bool is_refcount_carrying_function(enum bpf_func_id func_id) > +{ > + return func_id == BPF_FUNC_tcp_sock || > + func_id == BPF_FUNC_sk_fullsock; > +} > + > /* string representation of 'enum bpf_reg_type' */ > static const char * const reg_type_str[] = { > [NOT_INIT] = "?", > @@ -465,7 +460,8 @@ static void print_verifier_state(struct bpf_verifier_env *env, > if (t == PTR_TO_STACK) > verbose(env, ",call_%d", func(env, reg)->callsite); > } else { > - verbose(env, "(id=%d", reg->id); > + verbose(env, "(id=%d refcount_id=%d", reg->id, > + reg->refcount_id); > if (t != SCALAR_VALUE) > verbose(env, ",off=%d", reg->off); > if (type_is_pkt_pointer(t)) > @@ -2418,12 +2414,6 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno, > expected_type = PTR_TO_SOCKET; > if (type != expected_type) > goto err_type; > - if (meta->ptr_id || !reg->id) { > - verbose(env, "verifier internal error: mismatched references meta=%d, reg=%d\n", > - meta->ptr_id, reg->id); > - return -EFAULT; > - } > - meta->ptr_id = reg->id; > } else if (arg_type == ARG_PTR_TO_SPIN_LOCK) { > if (meta->func_id == BPF_FUNC_spin_lock) { > if (process_spin_lock(env, regno, true)) > @@ -2532,6 +2522,26 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno, > zero_size_allowed, meta); > } > > + if (reg->refcount_id) { > + if (meta->refcount_reg.refcount_id) { > + verbose(env, "verifier internal error: more than one arg with refcount_id R%d %u %u\n", > + regno, reg->refcount_id, > + meta->refcount_reg.refcount_id); > + return -EFAULT; > + } > + > + if (is_acquire_function(meta->func_id)) { > + verbose(env, > + "verifier internal error: func %s#%d taking an already refcount-ed arg R%d\n", > + func_id_name(meta->func_id), meta->func_id, > + regno); > + return -EFAULT; > + } > + > + meta->refcount_reg.id = reg->id; > + meta->refcount_reg.refcount_id = reg->refcount_id; > + } > + > return err; > err_type: > verbose(env, "R%d type=%s expected=%s\n", regno, > @@ -2799,19 +2809,20 @@ static void clear_all_pkt_pointers(struct bpf_verifier_env *env) > } > > static void release_reg_references(struct bpf_verifier_env *env, > - struct bpf_func_state *state, int id) > + struct bpf_func_state *state, > + int refcount_id) Could this just take id? release_reference could then also only take id. The way I see it, release_reg_references should clear all registers and stack spills where either id == id, or reference_id == id. In the current code that is given because id == reference_id if release_reference_state succeeds, but I find that harder to understand. > { > struct bpf_reg_state *regs = state->regs, *reg; > int i; > > for (i = 0; i < MAX_BPF_REG; i++) > - if (regs[i].id == id) > + if (regs[i].refcount_id == refcount_id) > mark_reg_unknown(env, regs, i); > > bpf_for_each_spilled_reg(i, state, reg) { > if (!reg) > continue; > - if (reg_is_refcounted(reg) && reg->id == id) > + if (reg->refcount_id == refcount_id) > __mark_reg_unknown(reg); > } > } > @@ -2819,16 +2830,21 @@ static void release_reg_references(struct bpf_verifier_env *env, > /* The pointer with the specified id has released its reference to kernel > * resources. Identify all copies of the same pointer and clear the reference. > */ > -static int release_reference(struct bpf_verifier_env *env, > - struct bpf_call_arg_meta *meta) > +static int release_reference(struct bpf_verifier_env *env, u32 id, > + u32 refcount_id) > { > struct bpf_verifier_state *vstate = env->cur_state; > + int err; > int i; > > + err = release_reference_state(cur_func(env), id); > + if (err) > + return err; > + > for (i = 0; i <= vstate->curframe; i++) > - release_reg_references(env, vstate->frame[i], meta->ptr_id); > + release_reg_references(env, vstate->frame[i], refcount_id); > > - return release_reference_state(cur_func(env), meta->ptr_id); > + return 0; > } > > static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn, > @@ -3093,7 +3109,8 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn > return err; > } > } else if (is_release_function(func_id)) { > - err = release_reference(env, &meta); > + err = release_reference(env, meta.refcount_reg.id, > + meta.refcount_reg.refcount_id); > if (err) { > verbose(env, "func %s#%d reference has not been acquired before\n", > func_id_name(func_id), func_id); > @@ -3156,6 +3173,7 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn > return id; > /* For release_reference() */ > regs[BPF_REG_0].id = id; > + regs[BPF_REG_0].refcount_id = id; > } else { > /* For mark_ptr_or_null_reg() */ > regs[BPF_REG_0].id = ++env->id_gen; > @@ -3170,6 +3188,9 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn > return -EINVAL; > } > > + if (is_refcount_carrying_function(func_id)) > + regs[BPF_REG_0].refcount_id = meta.refcount_reg.refcount_id; > + > do_refine_retval_range(regs, fn->ret_type, func_id, &meta); > > err = check_map_func_compatibility(env, meta.map_ptr, func_id); > @@ -4687,7 +4708,7 @@ static void mark_ptr_or_null_regs(struct bpf_verifier_state *vstate, u32 regno, > u32 id = regs[regno].id; > int i, j; > > - if (reg_is_refcounted_or_null(®s[regno]) && is_null) > + if (reg_is_refcounted(®s[regno]) && is_null) > release_reference_state(state, id); Since reg_is_refcounted() is true, release_reference_state should always succeed. Check the error code? > > for (i = 0; i < MAX_BPF_REG; i++) > -- > 2.17.1 > How about resetting reg->refcount_id in mark_ptr_or_null_reg as well? (Aside: where does the state pruning referenced by the comment in that function take place?)
On Fri, Mar 01, 2019 at 03:58:52PM +0000, Lorenz Bauer wrote: > On Fri, 1 Mar 2019 at 07:03, Martin KaFai Lau <kafai@fb.com> wrote: > > > > Lorenz Bauer [thanks!] reported that a ptr returned by bpf_tcp_sock(sk) > > can still be accessed after bpf_sk_release(sk). > > Both bpf_tcp_sock() and bpf_sk_fullsock() have the same issue. > > This patch addresses them together. > > > > A simple reproducer looks like this: > > > > sk = bpf_sk_lookup_tcp(); > > /* if (!sk) ... */ > > tp = bpf_tcp_sock(sk); > > /* if (!tp) ... */ > > bpf_sk_release(sk); > > snd_cwnd = tp->snd_cwnd; /* oops! The verifier does not complain. */ > > > > The problem is the verifier did not scrub the register's states of > > the tcp_sock ptr (tp) after bpf_sk_release(sk). > > > > [ Note that when calling bpf_tcp_sock(sk), the sk is not always > > refcount-acquired. e.g. bpf_tcp_sock(skb->sk). The verifier works > > fine for this case. ] > > > > Currently, the verifier does not track if a helper's return ptr (in REG_0) > > is "carry"-ing one of its argument's refcount status. To carry this info, > > the reg1->id needs to be stored in reg0. The reg0->id has already > > been used for NULL checking purpose. Hence, a new "refcount_id" > > is needed in "struct bpf_reg_state". > > > > With refcount_id, when bpf_sk_release(sk) is called, the verifier can scrub > > all reg states which has a refcount_id match. It is done with the changes > > in release_reg_references(). > > > > When acquiring and releasing a refcount, the reg->id is still used. > > Hence, we cannot do "bpf_sk_release(tp)" in the above reproducer > > example. > > > > Misc change notes: > > - With the new refcount_id, reg_is_refcounted() test can now be > > done with "reg->refcount_id && reg->id == reg->refcount_id" instead of > > testing the ptr type. > > > > The type_is_refcounted() and type_is_refcounted_or_null() > > are no longer needed, so removed. > > > > - An anonymous struct is added to bpf_call_arg_meta to store > > the reg->id and reg->refcount_id of the arg. Otherwise, > > they will be unavailable after check_helper_call() > > has cleared all CALLER_SAVED_REGS. > > > > - The check_func_arg() can only allow one refcount-ed arg. It is > > guaranteed by check_refcount_ok() which ensures at most one arg can be > > refcount-ed. Hence, it is a verifier internal error if >1 refcount arg > > found in check_func_arg(). > > > > - The check_func_arg() also complains if a "is_acquire_function(func_id)" > > helper is having a refcount-ed arg. No func_id is doing this now > > and should have been rejected earlier, so it is treated as > > verifier internal error also. > > > > - In check_func_arg(), the "!reg->id" check is removed under > > the ARG_PTR_TO_SOCKET case. It is because a PTR_TO_SOCKET > > can be obtained from bpf_sk_fullsock() and it does not > > take a refcount. The verifier will still complain during > > release_reference() but it does not treat it as a > > verifier internal error anymore. > > > > - In release_reference(), release_reference_state() is called > > first to ensure a match on "reg->id" can be found before > > scrubbing the reg states with release_reg_references(). > > > > Fixes: 655a51e536c0 ("bpf: Add struct bpf_tcp_sock and BPF_FUNC_tcp_sock") > > Cc: Lorenz Bauer <lmb@cloudflare.com> > > Reported-by: Lorenz Bauer <lmb@cloudflare.com> > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > --- > > include/linux/bpf_verifier.h | 9 ++++ > > kernel/bpf/verifier.c | 89 ++++++++++++++++++++++-------------- > > 2 files changed, 64 insertions(+), 34 deletions(-) > > > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > > index 69f7a3449eda..b7698d0534cb 100644 > > --- a/include/linux/bpf_verifier.h > > +++ b/include/linux/bpf_verifier.h > > @@ -66,6 +66,15 @@ struct bpf_reg_state { > > * same reference to the socket, to determine proper reference freeing. > > */ > > u32 id; > > + /* For PTR_TO_SOCKET and PTR_TO_TCP_SOCK, this ptr may not actually > > + * hold a refcount of a socket but instead it is a ptr > > + * returned from a helper which is based on its refcount-ed > > + * ptr argument (e.g. bpf_tcp_sock()). > > + * "refcount_id" stores which refcount-ed argument it > > + * originally derived from. When this original argument's > > + * refcount is released, this ptr will also be invalidated. > > + */ > > + u32 refcount_id; > > /* For scalar types (SCALAR_VALUE), this represents our knowledge of > > * the actual value. > > * For pointer types, this represents the variable part of the offset > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 1b9496c41383..682b7a6f0d2a 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -212,7 +212,10 @@ struct bpf_call_arg_meta { > > int access_size; > > s64 msize_smax_value; > > u64 msize_umax_value; > > - int ptr_id; > > + struct { > > + u32 id; > > + u32 refcount_id; > > + } refcount_reg; > > int func_id; > > }; > > > > @@ -346,19 +349,9 @@ static bool reg_type_may_be_null(enum bpf_reg_type type) > > type == PTR_TO_TCP_SOCK_OR_NULL; > > } > > > > -static bool type_is_refcounted(enum bpf_reg_type type) > > -{ > > - return type == PTR_TO_SOCKET; > > -} > > - > > -static bool type_is_refcounted_or_null(enum bpf_reg_type type) > > -{ > > - return type == PTR_TO_SOCKET || type == PTR_TO_SOCKET_OR_NULL; > > -} > > - > > static bool reg_is_refcounted(const struct bpf_reg_state *reg) > > { > > - return type_is_refcounted(reg->type); > > + return reg->refcount_id && reg->id == reg->refcount_id; > > } > > > > static bool reg_may_point_to_spin_lock(const struct bpf_reg_state *reg) > > @@ -367,14 +360,10 @@ static bool reg_may_point_to_spin_lock(const struct bpf_reg_state *reg) > > map_value_has_spin_lock(reg->map_ptr); > > } > > > > -static bool reg_is_refcounted_or_null(const struct bpf_reg_state *reg) > > -{ > > - return type_is_refcounted_or_null(reg->type); > > -} > > - > > static bool arg_type_is_refcounted(enum bpf_arg_type type) > > { > > - return type == ARG_PTR_TO_SOCKET; > > + return type == ARG_PTR_TO_SOCKET || > > + type == ARG_PTR_TO_SOCK_COMMON; > > } > > > > /* Determine whether the function releases some resources allocated by another > > @@ -392,6 +381,12 @@ static bool is_acquire_function(enum bpf_func_id func_id) > > func_id == BPF_FUNC_sk_lookup_udp; > > } > > > > +static bool is_refcount_carrying_function(enum bpf_func_id func_id) > > +{ > > + return func_id == BPF_FUNC_tcp_sock || > > + func_id == BPF_FUNC_sk_fullsock; > > +} > > + > > /* string representation of 'enum bpf_reg_type' */ > > static const char * const reg_type_str[] = { > > [NOT_INIT] = "?", > > @@ -465,7 +460,8 @@ static void print_verifier_state(struct bpf_verifier_env *env, > > if (t == PTR_TO_STACK) > > verbose(env, ",call_%d", func(env, reg)->callsite); > > } else { > > - verbose(env, "(id=%d", reg->id); > > + verbose(env, "(id=%d refcount_id=%d", reg->id, > > + reg->refcount_id); > > if (t != SCALAR_VALUE) > > verbose(env, ",off=%d", reg->off); > > if (type_is_pkt_pointer(t)) > > @@ -2418,12 +2414,6 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno, > > expected_type = PTR_TO_SOCKET; > > if (type != expected_type) > > goto err_type; > > - if (meta->ptr_id || !reg->id) { > > - verbose(env, "verifier internal error: mismatched references meta=%d, reg=%d\n", > > - meta->ptr_id, reg->id); > > - return -EFAULT; > > - } > > - meta->ptr_id = reg->id; > > } else if (arg_type == ARG_PTR_TO_SPIN_LOCK) { > > if (meta->func_id == BPF_FUNC_spin_lock) { > > if (process_spin_lock(env, regno, true)) > > @@ -2532,6 +2522,26 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno, > > zero_size_allowed, meta); > > } > > > > + if (reg->refcount_id) { > > + if (meta->refcount_reg.refcount_id) { > > + verbose(env, "verifier internal error: more than one arg with refcount_id R%d %u %u\n", > > + regno, reg->refcount_id, > > + meta->refcount_reg.refcount_id); > > + return -EFAULT; > > + } > > + > > + if (is_acquire_function(meta->func_id)) { > > + verbose(env, > > + "verifier internal error: func %s#%d taking an already refcount-ed arg R%d\n", > > + func_id_name(meta->func_id), meta->func_id, > > + regno); > > + return -EFAULT; > > + } > > + > > + meta->refcount_reg.id = reg->id; > > + meta->refcount_reg.refcount_id = reg->refcount_id; > > + } > > + > > return err; > > err_type: > > verbose(env, "R%d type=%s expected=%s\n", regno, > > @@ -2799,19 +2809,20 @@ static void clear_all_pkt_pointers(struct bpf_verifier_env *env) > > } > > > > static void release_reg_references(struct bpf_verifier_env *env, > > - struct bpf_func_state *state, int id) > > + struct bpf_func_state *state, > > + int refcount_id) > > Could this just take id? release_reference could then also only take > id. The way I see it, release_reg_references should clear all > registers and stack spills where either id == id, or reference_id == > id. > In the current code that is given because id == reference_id if > release_reference_state succeeds, but I find that harder to > understand. Agree. only id is needed. > > > { > > struct bpf_reg_state *regs = state->regs, *reg; > > int i; > > > > for (i = 0; i < MAX_BPF_REG; i++) > > - if (regs[i].id == id) > > + if (regs[i].refcount_id == refcount_id) > > mark_reg_unknown(env, regs, i); > > > > bpf_for_each_spilled_reg(i, state, reg) { > > if (!reg) > > continue; > > - if (reg_is_refcounted(reg) && reg->id == id) > > + if (reg->refcount_id == refcount_id) > > __mark_reg_unknown(reg); > > } > > } > > @@ -2819,16 +2830,21 @@ static void release_reg_references(struct bpf_verifier_env *env, > > /* The pointer with the specified id has released its reference to kernel > > * resources. Identify all copies of the same pointer and clear the reference. > > */ > > -static int release_reference(struct bpf_verifier_env *env, > > - struct bpf_call_arg_meta *meta) > > +static int release_reference(struct bpf_verifier_env *env, u32 id, > > + u32 refcount_id) > > { > > struct bpf_verifier_state *vstate = env->cur_state; > > + int err; > > int i; > > > > + err = release_reference_state(cur_func(env), id); > > + if (err) > > + return err; > > + > > for (i = 0; i <= vstate->curframe; i++) > > - release_reg_references(env, vstate->frame[i], meta->ptr_id); > > + release_reg_references(env, vstate->frame[i], refcount_id); > > > > - return release_reference_state(cur_func(env), meta->ptr_id); > > + return 0; > > } > > > > static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn, > > @@ -3093,7 +3109,8 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn > > return err; > > } > > } else if (is_release_function(func_id)) { > > - err = release_reference(env, &meta); > > + err = release_reference(env, meta.refcount_reg.id, > > + meta.refcount_reg.refcount_id); > > if (err) { > > verbose(env, "func %s#%d reference has not been acquired before\n", > > func_id_name(func_id), func_id); > > @@ -3156,6 +3173,7 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn > > return id; > > /* For release_reference() */ > > regs[BPF_REG_0].id = id; > > + regs[BPF_REG_0].refcount_id = id; > > } else { > > /* For mark_ptr_or_null_reg() */ > > regs[BPF_REG_0].id = ++env->id_gen; > > @@ -3170,6 +3188,9 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn > > return -EINVAL; > > } > > > > + if (is_refcount_carrying_function(func_id)) > > + regs[BPF_REG_0].refcount_id = meta.refcount_reg.refcount_id; > > + > > do_refine_retval_range(regs, fn->ret_type, func_id, &meta); > > > > err = check_map_func_compatibility(env, meta.map_ptr, func_id); > > @@ -4687,7 +4708,7 @@ static void mark_ptr_or_null_regs(struct bpf_verifier_state *vstate, u32 regno, > > u32 id = regs[regno].id; > > int i, j; > > > > - if (reg_is_refcounted_or_null(®s[regno]) && is_null) > > + if (reg_is_refcounted(®s[regno]) && is_null) > > release_reference_state(state, id); > > Since reg_is_refcounted() is true, release_reference_state should > always succeed. Check the error code? Check and log an internal verifier bug? Sure. It will need another change to pass down the "env" to here though. May be a WARN_ON_ONCE() instead. > > > > > for (i = 0; i < MAX_BPF_REG; i++) > > -- > > 2.17.1 > > > > How about resetting reg->refcount_id in mark_ptr_or_null_reg as well? I don't think so. release_reg_references() would not work then.
On Fri, Mar 01, 2019 at 09:07:48AM -0800, Martin Lau wrote: > > How about resetting reg->refcount_id in mark_ptr_or_null_reg as well? > I don't think so. release_reg_references() would not work then. After a second thought, 'reg->refcount_id = 0' should be done for the is_null case. I will respin.
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index 69f7a3449eda..b7698d0534cb 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -66,6 +66,15 @@ struct bpf_reg_state { * same reference to the socket, to determine proper reference freeing. */ u32 id; + /* For PTR_TO_SOCKET and PTR_TO_TCP_SOCK, this ptr may not actually + * hold a refcount of a socket but instead it is a ptr + * returned from a helper which is based on its refcount-ed + * ptr argument (e.g. bpf_tcp_sock()). + * "refcount_id" stores which refcount-ed argument it + * originally derived from. When this original argument's + * refcount is released, this ptr will also be invalidated. + */ + u32 refcount_id; /* For scalar types (SCALAR_VALUE), this represents our knowledge of * the actual value. * For pointer types, this represents the variable part of the offset diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 1b9496c41383..682b7a6f0d2a 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -212,7 +212,10 @@ struct bpf_call_arg_meta { int access_size; s64 msize_smax_value; u64 msize_umax_value; - int ptr_id; + struct { + u32 id; + u32 refcount_id; + } refcount_reg; int func_id; }; @@ -346,19 +349,9 @@ static bool reg_type_may_be_null(enum bpf_reg_type type) type == PTR_TO_TCP_SOCK_OR_NULL; } -static bool type_is_refcounted(enum bpf_reg_type type) -{ - return type == PTR_TO_SOCKET; -} - -static bool type_is_refcounted_or_null(enum bpf_reg_type type) -{ - return type == PTR_TO_SOCKET || type == PTR_TO_SOCKET_OR_NULL; -} - static bool reg_is_refcounted(const struct bpf_reg_state *reg) { - return type_is_refcounted(reg->type); + return reg->refcount_id && reg->id == reg->refcount_id; } static bool reg_may_point_to_spin_lock(const struct bpf_reg_state *reg) @@ -367,14 +360,10 @@ static bool reg_may_point_to_spin_lock(const struct bpf_reg_state *reg) map_value_has_spin_lock(reg->map_ptr); } -static bool reg_is_refcounted_or_null(const struct bpf_reg_state *reg) -{ - return type_is_refcounted_or_null(reg->type); -} - static bool arg_type_is_refcounted(enum bpf_arg_type type) { - return type == ARG_PTR_TO_SOCKET; + return type == ARG_PTR_TO_SOCKET || + type == ARG_PTR_TO_SOCK_COMMON; } /* Determine whether the function releases some resources allocated by another @@ -392,6 +381,12 @@ static bool is_acquire_function(enum bpf_func_id func_id) func_id == BPF_FUNC_sk_lookup_udp; } +static bool is_refcount_carrying_function(enum bpf_func_id func_id) +{ + return func_id == BPF_FUNC_tcp_sock || + func_id == BPF_FUNC_sk_fullsock; +} + /* string representation of 'enum bpf_reg_type' */ static const char * const reg_type_str[] = { [NOT_INIT] = "?", @@ -465,7 +460,8 @@ static void print_verifier_state(struct bpf_verifier_env *env, if (t == PTR_TO_STACK) verbose(env, ",call_%d", func(env, reg)->callsite); } else { - verbose(env, "(id=%d", reg->id); + verbose(env, "(id=%d refcount_id=%d", reg->id, + reg->refcount_id); if (t != SCALAR_VALUE) verbose(env, ",off=%d", reg->off); if (type_is_pkt_pointer(t)) @@ -2418,12 +2414,6 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno, expected_type = PTR_TO_SOCKET; if (type != expected_type) goto err_type; - if (meta->ptr_id || !reg->id) { - verbose(env, "verifier internal error: mismatched references meta=%d, reg=%d\n", - meta->ptr_id, reg->id); - return -EFAULT; - } - meta->ptr_id = reg->id; } else if (arg_type == ARG_PTR_TO_SPIN_LOCK) { if (meta->func_id == BPF_FUNC_spin_lock) { if (process_spin_lock(env, regno, true)) @@ -2532,6 +2522,26 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno, zero_size_allowed, meta); } + if (reg->refcount_id) { + if (meta->refcount_reg.refcount_id) { + verbose(env, "verifier internal error: more than one arg with refcount_id R%d %u %u\n", + regno, reg->refcount_id, + meta->refcount_reg.refcount_id); + return -EFAULT; + } + + if (is_acquire_function(meta->func_id)) { + verbose(env, + "verifier internal error: func %s#%d taking an already refcount-ed arg R%d\n", + func_id_name(meta->func_id), meta->func_id, + regno); + return -EFAULT; + } + + meta->refcount_reg.id = reg->id; + meta->refcount_reg.refcount_id = reg->refcount_id; + } + return err; err_type: verbose(env, "R%d type=%s expected=%s\n", regno, @@ -2799,19 +2809,20 @@ static void clear_all_pkt_pointers(struct bpf_verifier_env *env) } static void release_reg_references(struct bpf_verifier_env *env, - struct bpf_func_state *state, int id) + struct bpf_func_state *state, + int refcount_id) { struct bpf_reg_state *regs = state->regs, *reg; int i; for (i = 0; i < MAX_BPF_REG; i++) - if (regs[i].id == id) + if (regs[i].refcount_id == refcount_id) mark_reg_unknown(env, regs, i); bpf_for_each_spilled_reg(i, state, reg) { if (!reg) continue; - if (reg_is_refcounted(reg) && reg->id == id) + if (reg->refcount_id == refcount_id) __mark_reg_unknown(reg); } } @@ -2819,16 +2830,21 @@ static void release_reg_references(struct bpf_verifier_env *env, /* The pointer with the specified id has released its reference to kernel * resources. Identify all copies of the same pointer and clear the reference. */ -static int release_reference(struct bpf_verifier_env *env, - struct bpf_call_arg_meta *meta) +static int release_reference(struct bpf_verifier_env *env, u32 id, + u32 refcount_id) { struct bpf_verifier_state *vstate = env->cur_state; + int err; int i; + err = release_reference_state(cur_func(env), id); + if (err) + return err; + for (i = 0; i <= vstate->curframe; i++) - release_reg_references(env, vstate->frame[i], meta->ptr_id); + release_reg_references(env, vstate->frame[i], refcount_id); - return release_reference_state(cur_func(env), meta->ptr_id); + return 0; } static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn, @@ -3093,7 +3109,8 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn return err; } } else if (is_release_function(func_id)) { - err = release_reference(env, &meta); + err = release_reference(env, meta.refcount_reg.id, + meta.refcount_reg.refcount_id); if (err) { verbose(env, "func %s#%d reference has not been acquired before\n", func_id_name(func_id), func_id); @@ -3156,6 +3173,7 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn return id; /* For release_reference() */ regs[BPF_REG_0].id = id; + regs[BPF_REG_0].refcount_id = id; } else { /* For mark_ptr_or_null_reg() */ regs[BPF_REG_0].id = ++env->id_gen; @@ -3170,6 +3188,9 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn return -EINVAL; } + if (is_refcount_carrying_function(func_id)) + regs[BPF_REG_0].refcount_id = meta.refcount_reg.refcount_id; + do_refine_retval_range(regs, fn->ret_type, func_id, &meta); err = check_map_func_compatibility(env, meta.map_ptr, func_id); @@ -4687,7 +4708,7 @@ static void mark_ptr_or_null_regs(struct bpf_verifier_state *vstate, u32 regno, u32 id = regs[regno].id; int i, j; - if (reg_is_refcounted_or_null(®s[regno]) && is_null) + if (reg_is_refcounted(®s[regno]) && is_null) release_reference_state(state, id); for (i = 0; i < MAX_BPF_REG; i++)
Lorenz Bauer [thanks!] reported that a ptr returned by bpf_tcp_sock(sk) can still be accessed after bpf_sk_release(sk). Both bpf_tcp_sock() and bpf_sk_fullsock() have the same issue. This patch addresses them together. A simple reproducer looks like this: sk = bpf_sk_lookup_tcp(); /* if (!sk) ... */ tp = bpf_tcp_sock(sk); /* if (!tp) ... */ bpf_sk_release(sk); snd_cwnd = tp->snd_cwnd; /* oops! The verifier does not complain. */ The problem is the verifier did not scrub the register's states of the tcp_sock ptr (tp) after bpf_sk_release(sk). [ Note that when calling bpf_tcp_sock(sk), the sk is not always refcount-acquired. e.g. bpf_tcp_sock(skb->sk). The verifier works fine for this case. ] Currently, the verifier does not track if a helper's return ptr (in REG_0) is "carry"-ing one of its argument's refcount status. To carry this info, the reg1->id needs to be stored in reg0. The reg0->id has already been used for NULL checking purpose. Hence, a new "refcount_id" is needed in "struct bpf_reg_state". With refcount_id, when bpf_sk_release(sk) is called, the verifier can scrub all reg states which has a refcount_id match. It is done with the changes in release_reg_references(). When acquiring and releasing a refcount, the reg->id is still used. Hence, we cannot do "bpf_sk_release(tp)" in the above reproducer example. Misc change notes: - With the new refcount_id, reg_is_refcounted() test can now be done with "reg->refcount_id && reg->id == reg->refcount_id" instead of testing the ptr type. The type_is_refcounted() and type_is_refcounted_or_null() are no longer needed, so removed. - An anonymous struct is added to bpf_call_arg_meta to store the reg->id and reg->refcount_id of the arg. Otherwise, they will be unavailable after check_helper_call() has cleared all CALLER_SAVED_REGS. - The check_func_arg() can only allow one refcount-ed arg. It is guaranteed by check_refcount_ok() which ensures at most one arg can be refcount-ed. Hence, it is a verifier internal error if >1 refcount arg found in check_func_arg(). - The check_func_arg() also complains if a "is_acquire_function(func_id)" helper is having a refcount-ed arg. No func_id is doing this now and should have been rejected earlier, so it is treated as verifier internal error also. - In check_func_arg(), the "!reg->id" check is removed under the ARG_PTR_TO_SOCKET case. It is because a PTR_TO_SOCKET can be obtained from bpf_sk_fullsock() and it does not take a refcount. The verifier will still complain during release_reference() but it does not treat it as a verifier internal error anymore. - In release_reference(), release_reference_state() is called first to ensure a match on "reg->id" can be found before scrubbing the reg states with release_reg_references(). Fixes: 655a51e536c0 ("bpf: Add struct bpf_tcp_sock and BPF_FUNC_tcp_sock") Cc: Lorenz Bauer <lmb@cloudflare.com> Reported-by: Lorenz Bauer <lmb@cloudflare.com> Signed-off-by: Martin KaFai Lau <kafai@fb.com> --- include/linux/bpf_verifier.h | 9 ++++ kernel/bpf/verifier.c | 89 ++++++++++++++++++++++-------------- 2 files changed, 64 insertions(+), 34 deletions(-)