netdev - Re: [PATCH bpf-next v4 06/10] bpf: Track provenance for pointers formed from referenced PTR_TO_BTF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20211219215405.sf7xbzqhn5mcgpmq@apollo.legion>
Date:   Mon, 20 Dec 2021 03:24:05 +0530
From:   Kumar Kartikeya Dwivedi <memxor@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     bpf <bpf@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        netfilter-devel <netfilter-devel@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        Maxim Mikityanskiy <maximmi@...dia.com>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        Florian Westphal <fw@...len.de>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Toke Høiland-Jørgensen <toke@...hat.com>
Subject: Re: [PATCH bpf-next v4 06/10] bpf: Track provenance for pointers
 formed from referenced PTR_TO_BTF_ID

On Mon, Dec 20, 2021 at 02:56:45AM IST, Alexei Starovoitov wrote:
> On Mon, Dec 20, 2021 at 01:26:03AM +0530, Kumar Kartikeya Dwivedi wrote:
> > >
> > > The goal is clear now, but look at it differently:
> > > struct nf_conn *ct = bpf_xdp_ct_lookup(...);
> > > if (ct) {
> > >   struct nf_conn *master = ct->master;
> > >   struct net *net = ct->ct_net.net;
> > >
> > >   bpf_ct_release(ct);
> > >   master->status; // prevent this ?
> > >   net->ifindex;   // but allow this ?
> >
> > I think both will be prevented with the current logic, no?
> > net will be ct + offset, so if mark_btf_ld_reg writes PTR_TO_BTF_ID to dst_reg
> > for net, it will copy ct's reg's ref_obj_id to parent_ref_obj_id of dst_reg (net).
> > Then on release of ct, net's reg gets killed too since reg[ct]->ref_obj_id
> > matches its parent_ref_obj_id.
>
> Excatly, but it should be allowed.
> There is nothing wrong with 'net' access after ct_release.
>

Ok, I see your point. I'll just drop this patch in v5, and we'll revisit the
other pkt pointer thing when the patch is posted.

> [...]
> > Very interesting idea! I'm guessing we'll need something akin to bpf_timer
> > support, i.e. a dedicated type verified using BTF which can be embedded in
> > map_value? I'll be happy to work on enabling this.
>
> Thanks! Would be awesome.
>
> > One thought though (just confirming):
> > If user does map_value->saved_ct = ct, we have to ignore reference leak check
> > for ct's ref_id, but if they rewrite saved_ct, we would also have to unignore
> > it, correct?
>
> We cannot just ignore it :)
> I was thinking to borrow std::unique_ptr like semanitcs.
>
> struct nf_conn *ct = bpf_xdp_ct_lookup(...); // here ref checking logic tracks it as normal
> map_value->saved_ct = ct; // here it trasnfers the ref from Rx into map_value
> ct->status; // cannot be access here.
>
> It could look unnatural to typical C programmer, so we might need
> explicit std::move-like helper, so the assignment will be:
> bpf_move_ptr(&map_value->saved_ct, &ct); // same as map_value->saved_ct = ct; ct = NULL;
> ...
> bpf_move_ptr(&ct, &map_value->saved_ct); // would take the ownership back from the map
> // and the ref checking logic tracks 'ct' again as normal
>

Agreed, normal assignment syntax having those side effects is indeed awkward.

> > I think we can make this tracking easier by limiting to one bpf_ptr_to_btf
> > struct in map_value, then it can simply be part of ptr_to_map_value's reg_state.
>
> Possible. Hopefully such limitiation will not be needed.

Thanks for your review and feedback, Alexei! I'll address all points.

--
Kartikeya