lkml.org 
[lkml]   [2022]   [Nov]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RESEND][RFC][PATCH 2/3] bpf-lsm: Limit values that can be returned by security modules
    From
    Date
    On Thu, 2022-11-03 at 16:09 +0100, KP Singh wrote:
    > On Fri, Oct 28, 2022 at 6:55 PM Roberto Sassu
    > <roberto.sassu@huaweicloud.com> wrote:
    > > From: Roberto Sassu <roberto.sassu@huawei.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.

    Maybe it is possible to return specific values that lead to acquire
    more information/do actions that the eBPF program is not supposed to
    cause.

    I don't have a concrete example, so I will use the word you suggested.

    > > 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@vger.kernel.org
    > > Fixes: 9d3fdea789c8 ("bpf: lsm: Provide attachment points for BPF LSM programs")
    > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.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?

    The problem is that a new LSM_HOOK definition would apply to every LSM
    hook, while we need the ability to select subsets.

    I was thinking, but I didn't check yet, what about using BTF_ID_FLAGS,
    introducing a flag for each interval (<0, 0, 1, >1) and setting the
    appropriate flags for each LSM hook?

    Roberto

    > > 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
    > >

    \
     
     \ /
      Last update: 2022-11-04 16:30    [W:2.795 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site