netdev - Re: [PATCH v3 bpf-next 01/11] bpf: Move the PTR_TO_BTF_ID check to check_reg

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]
Date:   Tue, 22 Sep 2020 10:56:55 +0100
From:   Lorenz Bauer <lmb@...udflare.com>
To:     Martin KaFai Lau <kafai@...com>
Cc:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>,
        Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH v3 bpf-next 01/11] bpf: Move the PTR_TO_BTF_ID check to check_reg_type()

On Tue, 22 Sep 2020 at 08:04, Martin KaFai Lau <kafai@...com> wrote:
>
> check_reg_type() checks whether a reg can be used as an arg of a
> func_proto.  For PTR_TO_BTF_ID, the check is actually not
> completely done until the reg->btf_id is pointing to a
> kernel struct that is acceptable by the func_proto.
>
> Thus, this patch moves the btf_id check into check_reg_type().
> The compatible_reg_types[] usage is localized in check_reg_type() now.
>
> The "if (!btf_id) verbose(...); " is also removed since it won't happen.
>
> Signed-off-by: Martin KaFai Lau <kafai@...com>
> ---
>  kernel/bpf/verifier.c | 65 +++++++++++++++++++++++--------------------
>  1 file changed, 35 insertions(+), 30 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 15ab889b0a3f..3ce61c412ea0 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4028,20 +4028,29 @@ static const struct bpf_reg_types *compatible_reg_types[] = {
>         [__BPF_ARG_TYPE_MAX]            = NULL,
>  };
>
> -static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
> -                         const struct bpf_reg_types *compatible)
> +static int check_reg_type(struct bpf_verifier_env *env, u32 arg,
> +                         enum bpf_arg_type arg_type,
> +                         const struct bpf_func_proto *fn)

How about (env, regno, arg_type, expected_btf_id) instead? Otherwise
implementing sockmap update from iter with your current approach is
difficult for me. See resolve_map_arg_type, which now needs to resolve
expected_bpf_id.

See below for what I mean:

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2468533bc4a1..3a238a295c37 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3931,7 +3931,8 @@ static int int_ptr_type_to_size(enum bpf_arg_type type)

 static int resolve_map_arg_type(struct bpf_verifier_env *env,
                  const struct bpf_call_arg_meta *meta,
-                 enum bpf_arg_type *arg_type)
+                 enum bpf_arg_type *arg_type,
+                 u32 **expected_btf_id)
 {
     if (!meta->map_ptr) {
         /* kernel subsystem misconfigured verifier */
@@ -3943,7 +3944,8 @@ static int resolve_map_arg_type(struct
bpf_verifier_env *env,
     case BPF_MAP_TYPE_SOCKMAP:
     case BPF_MAP_TYPE_SOCKHASH:
         if (*arg_type == ARG_PTR_TO_MAP_VALUE) {
-            *arg_type = ARG_PTR_TO_SOCKET;
+            *arg_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON;
+            *expected_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON];
         } else {
             verbose(env, "invalid arg_type for sockmap/sockhash\n");
             return -EINVAL;
@@ -4044,11 +4046,9 @@ static const struct bpf_reg_types
*compatible_reg_types[] = {
     [__BPF_ARG_TYPE_MAX]        = NULL,
 };

-static int check_reg_type(struct bpf_verifier_env *env, u32 arg,
-              enum bpf_arg_type arg_type,
-              const struct bpf_func_proto *fn)
+static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
+              enum bpf_arg_type arg_type, u32 *expected_btf_id)
 {
-    u32 regno = BPF_REG_1 + arg;
     struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
     enum bpf_reg_type expected, type = reg->type;
     const struct bpf_reg_types *compatible;
@@ -4077,8 +4077,6 @@ static int check_reg_type(struct
bpf_verifier_env *env, u32 arg,

 found:
     if (type == PTR_TO_BTF_ID) {
-        u32 *expected_btf_id = fn->arg_btf_id[arg];
-
         if (!btf_struct_ids_match(&env->log, reg->off, reg->btf_id,
                       *expected_btf_id)) {
             verbose(env, "R%d is of type %s but %s is expected\n",
@@ -4105,6 +4103,7 @@ static int check_func_arg(struct
bpf_verifier_env *env, u32 arg,
     struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
     enum bpf_arg_type arg_type = fn->arg_type[arg];
     enum bpf_reg_type type = reg->type;
+    u32 *expected_btf_id = fn->arg_btf_id[arg];
     int err = 0;

     if (arg_type == ARG_DONTCARE)
@@ -4132,7 +4131,7 @@ static int check_func_arg(struct
bpf_verifier_env *env, u32 arg,
     if (arg_type == ARG_PTR_TO_MAP_VALUE ||
         arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE ||
         arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) {
-        err = resolve_map_arg_type(env, meta, &arg_type);
+        err = resolve_map_arg_type(env, meta, &arg_type, &expected_btf_id);
         if (err)
             return err;
     }
@@ -4143,7 +4142,7 @@ static int check_func_arg(struct
bpf_verifier_env *env, u32 arg,
          */
         goto skip_type_check;

-    err = check_reg_type(env, arg, arg_type, fn);
+    err = check_reg_type(env, regno, arg_type, expected_btf_id);
     if (err)
         return err;

-- 
2.25.1



--
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com