netdev - Re: [PATCH bpf-next 1/2] bpf: Allow bpf_map_lookup

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: <CAJ+HfNj4NgGQkJOEivuxuohA_+Fa98yD8EmY4acHQqymdUBA4g@mail.gmail.com>
Date:   Thu, 9 May 2019 13:48:13 +0200
From:   Björn Töpel <bjorn.topel@...il.com>
To:     Jonathan Lemon <jonathan.lemon@...il.com>
Cc:     Netdev <netdev@...r.kernel.org>,
        Björn Töpel <bjorn.topel@...el.com>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Alexei Starovoitov <ast@...nel.org>, kernel-team@...com
Subject: Re: [PATCH bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap

On Thu, 9 May 2019 at 01:07, Jonathan Lemon <jonathan.lemon@...il.com> wrote:
>
> Currently, the AF_XDP code uses a separate map in order to
> determine if an xsk is bound to a queue.  Instead of doing this,
> have bpf_map_lookup_elem() return a boolean indicating whether
> there is a valid entry at the map index.
>
> Signed-off-by: Jonathan Lemon <jonathan.lemon@...il.com>
> ---
>  kernel/bpf/verifier.c                             |  6 +++++-
>  kernel/bpf/xskmap.c                               |  2 +-
>  .../selftests/bpf/verifier/prevent_map_lookup.c   | 15 ---------------
>  3 files changed, 6 insertions(+), 17 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 7b05e8938d5c..a8b8ff9ecd90 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -2761,10 +2761,14 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env,
>          * appear.
>          */
>         case BPF_MAP_TYPE_CPUMAP:
> -       case BPF_MAP_TYPE_XSKMAP:
>                 if (func_id != BPF_FUNC_redirect_map)
>                         goto error;
>                 break;
> +       case BPF_MAP_TYPE_XSKMAP:
> +               if (func_id != BPF_FUNC_redirect_map &&
> +                   func_id != BPF_FUNC_map_lookup_elem)
> +                       goto error;
> +               break;
>         case BPF_MAP_TYPE_ARRAY_OF_MAPS:
>         case BPF_MAP_TYPE_HASH_OF_MAPS:
>                 if (func_id != BPF_FUNC_map_lookup_elem)
> diff --git a/kernel/bpf/xskmap.c b/kernel/bpf/xskmap.c
> index 686d244e798d..f6e49237979c 100644
> --- a/kernel/bpf/xskmap.c
> +++ b/kernel/bpf/xskmap.c
> @@ -154,7 +154,7 @@ void __xsk_map_flush(struct bpf_map *map)
>
>  static void *xsk_map_lookup_elem(struct bpf_map *map, void *key)
>  {
> -       return ERR_PTR(-EOPNOTSUPP);
> +       return !!__xsk_map_lookup_elem(map, *(u32 *)key);
>  }
>

Hmm, enabling lookups has some concerns, so we took the easy path;
simply disallowing it. Lookups (and returning a socket/fd) from
userspace might be expensive; allocating a new fd, and such, and on
the BPF side there's no XDP socket object (yet!).

Your patch makes the lookup return something else than a fd or socket.
The broader question is, inserting a socket fd and getting back a bool
-- is that ok from a semantic perspective? It's a kind of weird map.
Are there any other maps that behave in this way? It certainly makes
the XDP code easier, and you get somewhat better introspection into
the XSKMAP.

(bpf-next is closed, btw... :-))



Björn

>  static int xsk_map_update_elem(struct bpf_map *map, void *key, void *value,
> diff --git a/tools/testing/selftests/bpf/verifier/prevent_map_lookup.c b/tools/testing/selftests/bpf/verifier/prevent_map_lookup.c
> index bbdba990fefb..da7a4b37cb98 100644
> --- a/tools/testing/selftests/bpf/verifier/prevent_map_lookup.c
> +++ b/tools/testing/selftests/bpf/verifier/prevent_map_lookup.c
> @@ -28,21 +28,6 @@
>         .errstr = "cannot pass map_type 18 into func bpf_map_lookup_elem",
>         .prog_type = BPF_PROG_TYPE_SOCK_OPS,
>  },
> -{
> -       "prevent map lookup in xskmap",
> -       .insns = {
> -       BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> -       BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> -       BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> -       BPF_LD_MAP_FD(BPF_REG_1, 0),
> -       BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
> -       BPF_EXIT_INSN(),
> -       },
> -       .fixup_map_xskmap = { 3 },
> -       .result = REJECT,
> -       .errstr = "cannot pass map_type 17 into func bpf_map_lookup_elem",
> -       .prog_type = BPF_PROG_TYPE_XDP,
> -},
>  {
>         "prevent map lookup in stack trace",
>         .insns = {
> --
> 2.17.1
>