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

Message-ID: <CAADnVQJrZdC3f8SxxBqQK9Ov4Kcgao0enBNAhmwJuZPgxwjQUg@mail.gmail.com>
Date:   Wed, 30 Jun 2021 22:40:52 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     "David S. Miller" <davem@...emloft.net>,
        Martin KaFai Lau <kafai@...com>, Yonghong Song <yhs@...com>
Cc:     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 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'.
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?