| 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
| ||
|
Message-ID: <CAPhsuW5vx+JVBwpmU=fM0PerC_s_v=BfOj0-80W=p6YYbPem_A@mail.gmail.com>
Date: Tue, 6 Sep 2022 14:27:19 -0700
From: Song Liu <song@...nel.org>
To: Lorenzo Bianconi <lorenzo@...nel.org>
Cc: bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andrii@...nel.org>,
"David S . Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Eric Dumazet <edumazet@...gle.com>,
Paolo Abeni <pabeni@...hat.com>, pablo@...filter.org,
fw@...len.de, netfilter-devel@...r.kernel.org,
lorenzo.bianconi@...hat.com,
Jesper Dangaard Brouer <brouer@...hat.com>,
Toke Høiland-Jørgensen <toke@...hat.com>,
Kumar Kartikeya Dwivedi <memxor@...il.com>
Subject: Re: [PATCH v2 bpf-next 1/4] bpf: Add support for per-parameter
trusted args
On Mon, Sep 5, 2022 at 6:15 AM Lorenzo Bianconi <lorenzo@...nel.org> wrote:
>
> From: Kumar Kartikeya Dwivedi <memxor@...il.com>
>
> Similar to how we detect mem, size pairs in kfunc, teach verifier to
> treat __ref suffix on argument name to imply that it must be a trusted
> arg when passed to kfunc, similar to the effect of KF_TRUSTED_ARGS flag
> but limited to the specific parameter. This is required to ensure that
> kfunc that operate on some object only work on acquired pointers and not
> normal PTR_TO_BTF_ID with same type which can be obtained by pointer
> walking. Release functions need not specify such suffix on release
> arguments as they are already expected to receive one referenced
> argument.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@...il.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
LGTM:
Acked-by: Song Liu <song@...nel.org>
PS: IIUC, we use "refcounted" and "trusted" for the same concept. Shall we
just use __trusted? Pardon me if this has been discussed before.
Thanks,
Song
> ---
> Documentation/bpf/kfuncs.rst | 18 +++++++++++++++++
> kernel/bpf/btf.c | 39 ++++++++++++++++++++++++------------
> net/bpf/test_run.c | 9 +++++++--
> 3 files changed, 51 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
> index 781731749e55..a9d77d12fd0c 100644
> --- a/Documentation/bpf/kfuncs.rst
> +++ b/Documentation/bpf/kfuncs.rst
> @@ -72,6 +72,24 @@ argument as its size. By default, without __sz annotation, the size of the type
> of the pointer is used. Without __sz annotation, a kfunc cannot accept a void
> pointer.
>
> +2.2.2 __ref Annotation
> +----------------------
> +
> +This annotation is used to indicate that the argument is trusted, i.e. it will
> +be a pointer from an acquire function (defined later), and its offset will be
> +zero. This annotation has the same effect as the KF_TRUSTED_ARGS kfunc flag but
> +only on the parameter it is applied to. An example is shown below::
> +
> + void bpf_task_send_signal(struct task_struct *task__ref, int signal)
> + {
> + ...
> + }
> +
> +Here, bpf_task_send_signal will only act on trusted task_struct pointers, and
> +cannot be used on pointers obtained using pointer walking. This ensures that
> +caller always calls this kfunc on a task whose lifetime is guaranteed for the
> +duration of the call.
> +
> .. _BPF_kfunc_nodef:
>
> 2.3 Using an existing kernel function
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 903719b89238..7e273f949ee8 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6140,18 +6140,13 @@ static bool __btf_type_is_scalar_struct(struct bpf_verifier_log *log,
> return true;
> }
>
> -static bool is_kfunc_arg_mem_size(const struct btf *btf,
> - const struct btf_param *arg,
> - const struct bpf_reg_state *reg)
> +static bool btf_param_match_suffix(const struct btf *btf,
> + const struct btf_param *arg,
> + const char *suffix)
> {
> - int len, sfx_len = sizeof("__sz") - 1;
> - const struct btf_type *t;
> + int len, sfx_len = strlen(suffix);
> const char *param_name;
>
> - t = btf_type_skip_modifiers(btf, arg->type, NULL);
> - if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE)
> - return false;
> -
> /* In the future, this can be ported to use BTF tagging */
> param_name = btf_name_by_offset(btf, arg->name_off);
> if (str_is_empty(param_name))
> @@ -6160,10 +6155,26 @@ static bool is_kfunc_arg_mem_size(const struct btf *btf,
> if (len < sfx_len)
> return false;
> param_name += len - sfx_len;
> - if (strncmp(param_name, "__sz", sfx_len))
> + return !strncmp(param_name, suffix, sfx_len);
> +}
> +
> +static bool is_kfunc_arg_ref(const struct btf *btf,
> + const struct btf_param *arg)
> +{
> + return btf_param_match_suffix(btf, arg, "__ref");
> +}
> +
> +static bool is_kfunc_arg_mem_size(const struct btf *btf,
> + const struct btf_param *arg,
> + const struct bpf_reg_state *reg)
> +{
> + const struct btf_type *t;
> +
> + t = btf_type_skip_modifiers(btf, arg->type, NULL);
> + if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE)
> return false;
>
> - return true;
> + return btf_param_match_suffix(btf, arg, "__sz");
> }
>
> static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> @@ -6173,7 +6184,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> u32 kfunc_flags)
> {
> enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
> - bool rel = false, kptr_get = false, trusted_arg = false;
> + bool rel = false, kptr_get = false, kf_trusted_args = false;
> bool sleepable = false;
> struct bpf_verifier_log *log = &env->log;
> u32 i, nargs, ref_id, ref_obj_id = 0;
> @@ -6211,7 +6222,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> /* Only kfunc can be release func */
> rel = kfunc_flags & KF_RELEASE;
> kptr_get = kfunc_flags & KF_KPTR_GET;
> - trusted_arg = kfunc_flags & KF_TRUSTED_ARGS;
> + kf_trusted_args = kfunc_flags & KF_TRUSTED_ARGS;
> sleepable = kfunc_flags & KF_SLEEPABLE;
> }
>
> @@ -6222,6 +6233,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> enum bpf_arg_type arg_type = ARG_DONTCARE;
> u32 regno = i + 1;
> struct bpf_reg_state *reg = ®s[regno];
> + bool trusted_arg = false;
>
> t = btf_type_skip_modifiers(btf, args[i].type, NULL);
> if (btf_type_is_scalar(t)) {
> @@ -6240,6 +6252,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> /* Check if argument must be a referenced pointer, args + i has
> * been verified to be a pointer (after skipping modifiers).
> */
> + trusted_arg = kf_trusted_args || is_kfunc_arg_ref(btf, args + i);
> if (is_kfunc && trusted_arg && !reg->ref_obj_id) {
> bpf_log(log, "R%d must be referenced\n", regno);
> return -EINVAL;
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index 25d8ecf105aa..b735accf8750 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -691,7 +691,11 @@ noinline void bpf_kfunc_call_test_mem_len_fail2(u64 *mem, int len)
> {
> }
>
> -noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p)
> +noinline void bpf_kfunc_call_test_trusted(struct prog_test_ref_kfunc *p)
> +{
> +}
> +
> +noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p__ref)
> {
> }
>
> @@ -722,7 +726,8 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_test_fail3)
> BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_pass1)
> BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail1)
> BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail2)
> -BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref, KF_TRUSTED_ARGS)
> +BTF_ID_FLAGS(func, bpf_kfunc_call_test_trusted, KF_TRUSTED_ARGS)
> +BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref)
> BTF_ID_FLAGS(func, bpf_kfunc_call_test_destructive, KF_DESTRUCTIVE)
> BTF_SET8_END(test_sk_check_kfunc_ids)
>
> --
> 2.37.3
>
Powered by blists - more mailing lists