linux-kernel - Re: [PATCH bpf-next v3 4/5] bpf: Add support for writing to nf

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220820002124.g3d5fud5klvrkjil@kashmir.localdomain>
Date:   Fri, 19 Aug 2022 18:21:24 -0600
From:   Daniel Xu <dxu@...uu.xyz>
To:     Kumar Kartikeya Dwivedi <memxor@...il.com>
Cc:     bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net,
        andrii@...nel.org, pablo@...filter.org, fw@...len.de,
        toke@...nel.org, martin.lau@...ux.dev,
        netfilter-devel@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v3 4/5] bpf: Add support for writing to
 nf_conn:mark

Hi Kumar,

On Sat, Aug 20, 2022 at 01:46:04AM +0200, Kumar Kartikeya Dwivedi wrote:
> On Sat, 20 Aug 2022 at 01:23, Daniel Xu <dxu@...uu.xyz> wrote:
[...]
> > +static int tc_cls_act_btf_struct_access(struct bpf_verifier_log *log,
> > +                                       const struct btf *btf,
> > +                                       const struct btf_type *t, int off,
> > +                                       int size, enum bpf_access_type atype,
> > +                                       u32 *next_btf_id,
> > +                                       enum bpf_type_flag *flag)
> > +{
> > +       btf_struct_access_t sa;
> > +
> > +       if (atype == BPF_READ)
> > +               return btf_struct_access(log, btf, t, off, size, atype, next_btf_id,
> > +                                        flag);
> > +
> > +       sa = READ_ONCE(nf_conntrack_btf_struct_access);
> 
> This looks unsafe. How do you prevent this race?
> 
> CPU 0                                              CPU 1
> sa = READ_ONCE(nf_ct_bsa);
> 
> delete_module("nf_conntrack", ..);
> 
> WRITE_ONCE(nf_ct_bsa, NULL);
>                                                          // finishes
> successfully
> if (sa)
>     return sa(...); // oops
> 
> i.e. what keeps the module alive while we execute its callback?
> 
> Using a mutex is one way (as I suggested previously), either you
> acquire it before unload, or after. If after, you see cb as NULL,
> otherwise if unload is triggered concurrently it waits to acquire the
> mutex held by us. Unsetting the cb would be the first thing the module
> would do.
> 
> You can also hold a module reference, but then you must verify it is
> nf_conntrack's BTF before using btf_try_get_module.
> But _something_ needs to be done to prevent the module from going away
> while we execute its code.

I think I somehow convinced myself that nf_conntrack_core.o is always
compiled in. Due to some of the garbage collection semantics I saw in
the code.

Lemme take a closer look (for learning I guess). Mutex is probably
safest bet.

[...]

Thanks,
Daniel