On Mon, Sep 07, 2020 at 09:57:06AM +0100, Lorenz Bauer wrote: > On Sun, 6 Sep 2020 at 23:40, Martin KaFai Lau <kafai@xxxxxx> wrote: > > > > On Fri, Sep 04, 2020 at 10:58:59AM +0100, Lorenz Bauer wrote: > > > Tracing programs can derive struct sock pointers from a variety > > > of sources, e.g. a bpf_iter for sk_storage maps receives one as > > > part of the context. It's desirable to be able to pass these to > > > functions that expect PTR_TO_SOCKET. For example, it enables us > > > to insert such a socket into a sockmap via map_elem_update. > > > > > > Teach the verifier that a PTR_TO_BTF_ID for a struct sock is > > > equivalent to PTR_TO_SOCKET. There is one hazard here: > > > bpf_sk_release also takes a PTR_TO_SOCKET, but expects it to be > > > refcounted. Since this isn't the case for pointers derived from > > > BTF we must prevent them from being passed to the function. > > > Luckily, we can simply check that the ref_obj_id is not zero > > > in release_reference, and return an error otherwise. > > > > > > Signed-off-by: Lorenz Bauer <lmb@xxxxxxxxxxxxxx> > > > --- > > > kernel/bpf/verifier.c | 61 +++++++++++++++++++++++++------------------ > > > 1 file changed, 36 insertions(+), 25 deletions(-) > > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > index b4e9c56b8b32..509754c3aa7d 100644 > > > --- a/kernel/bpf/verifier.c > > > +++ b/kernel/bpf/verifier.c > > > @@ -3908,6 +3908,9 @@ static int resolve_map_arg_type(struct bpf_verifier_env *env, > > > return 0; > > > } > > > > > > +BTF_ID_LIST(btf_fullsock_ids) > > > +BTF_ID(struct, sock) > > It may be fine for the sockmap iter case to treat the "struct sock" BTF_ID > > as a fullsock (i.e. PTR_TO_SOCKET). > > I think it's unsafe even for the sockmap iter. Since it's a tracing > prog there might > be other ways for it to obtain a struct sock * in the future. > > > This is a generic verifier change though. For tracing, it is not always the > > case. It cannot always assume that the "struct sock *" in the function being > > traced is always a fullsock. > > Yes, I see, thanks for reminding me. What a footgun. I think the > problem boils down > to the fact that we can't express "this is a full socket" in BTF, > since there is no such > type in the kernel. > > Which makes me wonder: how do tracing programs deal with struct sock* > that really > is a request sock or something? PTR_TO_BTF_ID is handled differently, by BPF_PROBE_MEM, to take care of cases like this. bpf_jit_comp.c has some more details. [ ... ] > > > @@ -4561,6 +4569,9 @@ static int release_reference(struct bpf_verifier_env *env, > > > int err; > > > int i; > > > > > > + if (!ref_obj_id) > > > + return -EINVAL; > > hmm...... Is it sure this is needed? If it was, it seems there was > > an existing bug in release_reference_state() below which could not catch > > the case where "bpf_sk_release()" is called on a pointer that has no > > reference acquired before. > > Since sk_release takes a PTR_TO_SOCKET, it's possible to pass a tracing > struct sock * to it after this patch. Adding this check prevents the > release from > succeeding. Not all existing PTR_TO_SOCK_COMMON takes a reference also. Does it mean all these existing cases are broken? For example, bpf_sk_release(__sk_buff->sk) is allowed now? > > > > > Can you write a verifier test to demonstrate the issue? > > There is a selftest in this series that ensures calling sk_release > doesn't work, which exercises this.b I am not sure what Patch 4 of this series is testing. bpf_sk_release is not even available in bpf tracing iter program. There are ref tracking tests in tools/testing/selftests/bpf/verifier/ref_tracking.c. Please add all ref count related test there to catch the issue.
