netdev - Re: [PATCH v3 bpf-next 1/8] bpf: Introduce bpf timers.

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: <20210701153414.5kxste77mejnv4yp@ast-mbp.dhcp.thefacebook.com>
Date:   Thu, 1 Jul 2021 08:34:14 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Martin KaFai Lau <kafai@...com>, Yonghong Song <yhs@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Network Development <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v3 bpf-next 1/8] bpf: Introduce bpf timers.

On Thu, Jul 01, 2021 at 01:51:04PM +0200, Toke Høiland-Jørgensen wrote:
> Alexei Starovoitov <alexei.starovoitov@...il.com> writes:
> 
> > On Wed, Jun 23, 2021 at 7:25 PM Alexei Starovoitov
> > <alexei.starovoitov@...il.com> wrote:
> >>
> >> The bpf_timer_init() helper is receiving hidden 'map' argument and
> > ...
> >> +               if (insn->imm == BPF_FUNC_timer_init) {
> >> +                       aux = &env->insn_aux_data[i + delta];
> >> +                       if (bpf_map_ptr_poisoned(aux)) {
> >> +                               verbose(env, "bpf_timer_init abusing map_ptr\n");
> >> +                               return -EINVAL;
> >> +                       }
> >> +                       map_ptr = BPF_MAP_PTR(aux->map_ptr_state);
> >> +                       {
> >> +                               struct bpf_insn ld_addrs[2] = {
> >> +                                       BPF_LD_IMM64(BPF_REG_3, (long)map_ptr),
> >> +                               };
> >
> > After a couple of hours of ohh so painful debugging I realized that this
> > approach doesn't work for inner maps. Duh.
> > For inner maps it remembers inner_map_meta which is a template
> > of inner map.
> > Then bpf_timer_cb() passes map ptr into timer callback and if it tries
> > to do map operations on it the inner_map_meta->ops will be valid,
> > but the struct bpf_map doesn't have the actual data.
> > So to support map-in-map we need to require users to pass map pointer
> > explicitly into bpf_timer_init().
> > Unfortunately the verifier cannot guarantee that bpf timer field inside
> > map element is from the same map that is passed as a map ptr.
> > The verifier can check that they're equivalent from safety pov
> > via bpf_map_meta_equal(), so common user mistakes will be caught by it.
> > Still not pretty that it's partially on the user to do:
> > bpf_timer_init(timer, CLOCK, map);
> > with 'timer' matching the 'map'.
> 
> The implication being that if they don't match, the callback will just
> get a different argument and it'll be up to the developer to deal with
> any bugs arising from that?

Right. The kernel won't crash, of course.

> > Another option is to drop 'map' arg from timer callback,
> > but the usability of the callback will suffer. The inner maps
> > will be quite painful to use from it.
> > Anyway I'm going with explicit 'map' arg in the next respin.
> > Other ideas?
> 
> So the problem here is that the inner map pointer is not known at
> verification time but only at runtime? Could the verifier inject code to

yep.

> always spill inner map pointers to a known area of the stack after a
> map-in-map lookup, and then just load them back from there when needed?

interesting idea. That made me thinking that the verifier has
"map_lookup tracking" ability with increasing reg->id.
Since in some cases we had to distinguish that
val1 = map_lookup(map1, key1);
val2 = map_lookup(map1, key1);
val1 != val2, though they could be from the same map and key.
Maybe building on top of that feature will address the map vs timer
equivalence issue.

> Not sure that would be worth the complexity (and overhead!), though;
> having to supply an explicit callback arg is not that uncommon a pattern
> after all...
> 
> -Toke
> 

--