Message-ID: <CACYkzJ5gFu5a-NoKFD6XFNYMDyP+iPon=kHMimJybmNexbhAPg@mail.gmail.com>
Date:   Thu, 3 Nov 2022 16:09:56 +0100
From:   KP Singh <kpsingh@...nel.org>
To:     Roberto Sassu <roberto.sassu@...weicloud.com>
Cc:     ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
        martin.lau@...ux.dev, song@...nel.org, yhs@...com,
        john.fastabend@...il.com, sdf@...gle.com, haoluo@...gle.com,
        jolsa@...nel.org, mykolal@...com, revest@...omium.org,
        jackmanb@...omium.org, shuah@...nel.org, paul@...l-moore.com,
        casey@...aufler-ca.com, zohar@...ux.ibm.com, bpf@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        linux-integrity@...r.kernel.org, linux-kselftest@...r.kernel.org,
        linux-kernel@...r.kernel.org, nicolas.bouchinet@...p-os.org
Subject: Re: [RESEND][RFC][PATCH 2/3] bpf-lsm: Limit values that can be
 returned by security modules

On Fri, Oct 28, 2022 at 6:55 PM Roberto Sassu
<roberto.sassu@...weicloud.com> wrote:
>
> From: Roberto Sassu <roberto.sassu@...wei.com>
>
> BPF LSM defines a bpf_lsm_*() function for each LSM hook, so that
> security modules can define their own implementation for the desired hooks.
>
> Unfortunately, BPF LSM does not restrict which values security modules can
> return (for non-void LSM hooks). Security modules might follow the
> conventions stated in include/linux/lsm_hooks.h, or put arbitrary values.
>
> This could cause big troubles, as the kernel is not ready to handle
> possibly malicious return values from LSMs. Until now, it was not the

I am not sure I would call this malicious. This would be incorrect, if
someone is writing a BPF LSM program they already have the powers
to willingly do a lot of malicious stuff.

It's about unknowingly returning values that can break the system.

> case, as each LSM is carefully reviewed and it won't be accepted if it
> does not meet the return value conventions.
>
> The biggest problem is when an LSM returns a positive value, instead of a
> negative value, as it could be converted to a pointer. Since such pointer
> escapes the IS_ERR() check, its use later in the code can cause
> unpredictable consequences (e.g. invalid memory access).
>
> Another problem is returning zero when an LSM is supposed to have done some
> operations. For example, the inode_init_security hook expects that their
> implementations return zero only if they set the name and value of the new
> xattr to be added to the new inode. Otherwise, other kernel subsystems
> might encounter unexpected conditions leading to a crash (e.g.
> evm_protected_xattr_common() getting NULL as argument).
>
> Finally, there are LSM hooks which are supposed to return just one as
> positive value, or non-negative values. Also in these cases, although it
> seems less critical, it is safer to return to callers of the LSM
> infrastructure more precisely what they expect.
>
> As eBPF allows code outside the kernel to run, it is its responsibility
> to ensure that only expected values are returned to LSM infrastructure
> callers.
>
> Create four new BTF ID sets, respectively for hooks that can return
> positive values, only one as positive value, that cannot return zero, and
> that cannot return negative values. Create also corresponding functions to
> check if the hook a security module is attached to belongs to one of the
> defined sets.
>
> Finally, check in the eBPF verifier the value returned by security modules
> for each attached LSM hook, and return -EINVAL (the security module cannot
> run) if the hook implementation does not satisfy the hook return value
> policy.
>
> Cc: stable@...r.kernel.org
> Fixes: 9d3fdea789c8 ("bpf: lsm: Provide attachment points for BPF LSM programs")
> Signed-off-by: Roberto Sassu <roberto.sassu@...wei.com>
> ---
>  include/linux/bpf_lsm.h | 24 ++++++++++++++++++
>  kernel/bpf/bpf_lsm.c    | 56 +++++++++++++++++++++++++++++++++++++++++
>  kernel/bpf/verifier.c   | 35 +++++++++++++++++++++++---
>  3 files changed, 112 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
> index 4bcf76a9bb06..cd38aca4cfc0 100644
> --- a/include/linux/bpf_lsm.h
> +++ b/include/linux/bpf_lsm.h
> @@ -28,6 +28,10 @@ int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
>                         const struct bpf_prog *prog);
>
>  bool bpf_lsm_is_sleepable_hook(u32 btf_id);
> +bool bpf_lsm_can_ret_pos_value(u32 btf_id);
> +bool bpf_lsm_can_ret_only_one_as_pos_value(u32 btf_id);
> +bool bpf_lsm_cannot_ret_zero(u32 btf_id);
> +bool bpf_lsm_cannot_ret_neg_value(u32 btf_id);
>

This does not need to be exported to the rest of the kernel. Please
have this logic in bpf_lsm.c and export a single verify function.

Also, these really don't need to be such scattered logic, Could we
somehow encode this into the LSM_HOOK definition?

>  static inline struct bpf_storage_blob *bpf_inode(
>         const struct inode *inode)
> @@ -51,6 +55,26 @@ static inline bool bpf_lsm_is_sleepable_hook(u32 btf_id)
>         return false;
>  }
>
> +static inline bool bpf_lsm_can_ret_pos_value(u32 btf_id)
> +{
> +       return false;
> +}
> +
> +static inline bool bpf_lsm_can_ret_only_one_as_pos_value(u32 btf_id)
> +{
> +       return false;
> +}
> +
> +static inline bool bpf_lsm_cannot_ret_zero(u32 btf_id)
> +{
> +       return false;
> +}
> +
> +static inline bool bpf_lsm_cannot_ret_neg_value(u32 btf_id)
> +{
> +       return false;
> +}
> +
>  static inline int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
>                                       const struct bpf_prog *prog)
>  {
> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> index d6c9b3705f24..3dcb70b2f978 100644
> --- a/kernel/bpf/bpf_lsm.c
> +++ b/kernel/bpf/bpf_lsm.c
> @@ -348,6 +348,62 @@ bool bpf_lsm_is_sleepable_hook(u32 btf_id)
>         return btf_id_set_contains(&sleepable_lsm_hooks, btf_id);
>  }
>
> +/* The set of hooks which are allowed to return a positive value. */
> +BTF_SET_START(pos_ret_value_lsm_hooks)
> +BTF_ID(func, bpf_lsm_vm_enough_memory)
> +BTF_ID(func, bpf_lsm_inode_getsecurity)
> +BTF_ID(func, bpf_lsm_inode_listsecurity)
> +BTF_ID(func, bpf_lsm_inode_need_killpriv)
> +BTF_ID(func, bpf_lsm_inode_copy_up_xattr)
> +BTF_ID(func, bpf_lsm_getprocattr)
> +BTF_ID(func, bpf_lsm_setprocattr)
> +BTF_ID(func, bpf_lsm_xfrm_state_pol_flow_match)
> +BTF_ID(func, bpf_lsm_key_getsecurity)
> +BTF_ID(func, bpf_lsm_ismaclabel)
> +BTF_ID(func, bpf_lsm_audit_rule_known)
> +BTF_ID(func, bpf_lsm_audit_rule_match)
> +BTF_SET_END(pos_ret_value_lsm_hooks)
> +
> +bool bpf_lsm_can_ret_pos_value(u32 btf_id)
> +{
> +       return btf_id_set_contains(&pos_ret_value_lsm_hooks, btf_id);
> +}
> +
> +BTF_SET_START(one_ret_value_lsm_hooks)
> +BTF_ID(func, bpf_lsm_vm_enough_memory)
> +BTF_ID(func, bpf_lsm_inode_copy_up_xattr)
> +BTF_ID(func, bpf_lsm_xfrm_state_pol_flow_match)
> +BTF_ID(func, bpf_lsm_ismaclabel)
> +BTF_ID(func, bpf_lsm_audit_rule_known)
> +BTF_ID(func, bpf_lsm_audit_rule_match)
> +BTF_SET_END(one_ret_value_lsm_hooks)
> +
> +bool bpf_lsm_can_ret_only_one_as_pos_value(u32 btf_id)
> +{
> +       return btf_id_set_contains(&one_ret_value_lsm_hooks, btf_id);
> +}
> +
> +/* The set of hooks which are not allowed to return zero. */
> +BTF_SET_START(not_zero_ret_value_lsm_hooks)
> +BTF_ID(func, bpf_lsm_inode_init_security)
> +BTF_SET_END(not_zero_ret_value_lsm_hooks)
> +
> +bool bpf_lsm_cannot_ret_zero(u32 btf_id)
> +{
> +       return btf_id_set_contains(&not_zero_ret_value_lsm_hooks, btf_id);
> +}
> +
> +/* The set of hooks which are not allowed to return a negative value. */
> +BTF_SET_START(not_neg_ret_value_lsm_hooks)
> +BTF_ID(func, bpf_lsm_vm_enough_memory)
> +BTF_ID(func, bpf_lsm_audit_rule_known)
> +BTF_SET_END(not_neg_ret_value_lsm_hooks)
> +
> +bool bpf_lsm_cannot_ret_neg_value(u32 btf_id)
> +{
> +       return btf_id_set_contains(&not_neg_ret_value_lsm_hooks, btf_id);
> +}
> +
>  const struct bpf_prog_ops lsm_prog_ops = {
>  };
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 7f0a9f6cb889..099c1bf88fed 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -10623,9 +10623,38 @@ static int check_return_code(struct bpf_verifier_env *env)
>
>         case BPF_PROG_TYPE_LSM:
>                 if (env->prog->expected_attach_type != BPF_LSM_CGROUP) {
> -                       /* Regular BPF_PROG_TYPE_LSM programs can return
> -                        * any value.
> -                        */
> +                       /* < 0 */
> +                       if (tnum_in(tnum_range((u64)(~0) << 31, (u64)(~0)), reg->var_off)) {
> +                               if (bpf_lsm_cannot_ret_neg_value(env->prog->aux->attach_btf_id)) {
> +                                       verbose(env, "Invalid R0, cannot return negative value\n");
> +                                       return -EINVAL;
> +                               }
> +                       /* = 0 */
> +                       } else if (tnum_equals_const(reg->var_off, 0)) {
> +                               if (bpf_lsm_cannot_ret_zero(env->prog->aux->attach_btf_id)) {
> +                                       verbose(env, "Invalid R0, cannot return zero value\n");
> +                                       return -EINVAL;
> +                               }
> +                       /* = 1 */
> +                       } else if (tnum_equals_const(reg->var_off, 1)) {
> +                               if (!bpf_lsm_can_ret_pos_value(env->prog->aux->attach_btf_id)) {
> +                                       verbose(env, "Invalid R0, cannot return positive value\n");
> +                                       return -EINVAL;
> +                               }
> +                       /* > 1 */
> +                       } else {
> +                               if (!bpf_lsm_can_ret_pos_value(env->prog->aux->attach_btf_id)) {
> +                                       verbose(env, "Invalid R0, cannot return positive value\n");
> +                                       return -EINVAL;
> +                               }
> +
> +                               if (bpf_lsm_can_ret_only_one_as_pos_value(env->prog->aux->attach_btf_id)) {
> +                                       verbose(env,
> +                                               "Invalid R0, can return only one as positive value\n");
> +                                       return -EINVAL;
> +                               }
> +                       }
> +
>                         return 0;
>                 }
>                 if (!env->prog->aux->attach_func_proto->type) {
> --
> 2.25.1
>

Powered by blists - more mailing lists