Message ID | 20221028165423.386151-2-roberto.sassu@huaweicloud.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp939307wru; Fri, 28 Oct 2022 10:00:43 -0700 (PDT) X-Google-Smtp-Source: AMsMyM7ADOFrtWCPNX8kLUpTWE1VCKsKlDl0ZR6Npg3O6SUfYzleOHVagsMfPMk8RuNwa/YL+KHh X-Received: by 2002:a05:6402:1cc1:b0:45c:3a90:9499 with SMTP id ds1-20020a0564021cc100b0045c3a909499mr379229edb.61.1666976443324; Fri, 28 Oct 2022 10:00:43 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666976443; cv=none; d=google.com; s=arc-20160816; b=X9SG8m9cHipsPBu6jxXpbNma3zzTrIKbMRJnJUZDfBWtV4EPEsMqw7KyYbvB6NAmG7 OT+7hdSrRzMfxvRPWLR19qTPXu7oFe8A7YiztSVkLPbpVsZ9lma2apnl9T1BmHBykUPA MDxbVt/6YB5Jac454h9JaIXqJK0PHIoz1Iz4NGuAKxhpEEigPm9QQOn2KCWa38aGo9Sa f44lKGvjh+Zz9g4mDMlbJPE1aG1lH5Ki1k1YXqsvkKMiZPRMNfMbx2aaPo3lyyBs2Cco T375p6FtKyU9gnsqKParHgwppr7+n/KWANdYqLC7nD/kAin1mg8ehNmgljGcITQL9vGO MT3A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from; bh=R8NIPAQzFpFz2oXY0aNe9MDDrW69no6hQWeW0z3H2vU=; b=Fo2qi+EMl3L4CwZcg+SnZ/TLj2Wca5TDGqR0wOjqWFdiW+FaDjUJY8QGe1tBdKHtwC rsiB2QAaBvetrz+JEP7truNeVLI0Hcw+rxx1oAnhRp+WAgGafi1WOg0KhV7bTbLknmnp x3LJ2gm/HRf5AeKOiJt/jl2CDFYLbU/q043Ng26IauTs14LEI0Zf/1+YjcW/I6NSrnDu fz6TP0or19PZJ1xhbFOoirb+uABbLp46UA28syNJmGP2nwjaDabgcn0iLpeyb4K/dsYB W//f7daRXRhw8hvphHbeKAOOeYzLXNApge7dSL3dDrqRtLrDQ4WUoHX95uZv5FQxTWOF qdmQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id mp41-20020a1709071b2900b0073da5c8de1asi5919641ejc.178.2022.10.28.10.00.18; Fri, 28 Oct 2022 10:00:43 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229822AbiJ1Q4M (ORCPT <rfc822;norden.jeffspam@gmail.com> + 99 others); Fri, 28 Oct 2022 12:56:12 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33248 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230297AbiJ1Qze (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 28 Oct 2022 12:55:34 -0400 Received: from frasgout12.his.huawei.com (frasgout12.his.huawei.com [14.137.139.154]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A97931C4924; Fri, 28 Oct 2022 09:55:27 -0700 (PDT) Received: from mail02.huawei.com (unknown [172.18.147.227]) by frasgout12.his.huawei.com (SkyGuard) with ESMTP id 4MzT6k49qPz9xtVb; Sat, 29 Oct 2022 00:49:50 +0800 (CST) Received: from huaweicloud.com (unknown [10.204.63.22]) by APP1 (Coremail) with SMTP id LxC2BwBX9XFOCVxj63ccAA--.45991S3; Fri, 28 Oct 2022 17:55:03 +0100 (CET) From: Roberto Sassu <roberto.sassu@huaweicloud.com> To: ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org, martin.lau@linux.dev, song@kernel.org, yhs@fb.com, john.fastabend@gmail.com, kpsingh@kernel.org, sdf@google.com, haoluo@google.com, jolsa@kernel.org, mykolal@fb.com, revest@chromium.org, jackmanb@chromium.org, shuah@kernel.org, paul@paul-moore.com, casey@schaufler-ca.com, zohar@linux.ibm.com Cc: bpf@vger.kernel.org, linux-security-module@vger.kernel.org, linux-integrity@vger.kernel.org, linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org, nicolas.bouchinet@clip-os.org Subject: [RESEND][RFC][PATCH 2/3] bpf-lsm: Limit values that can be returned by security modules Date: Fri, 28 Oct 2022 18:54:22 +0200 Message-Id: <20221028165423.386151-2-roberto.sassu@huaweicloud.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20221028165423.386151-1-roberto.sassu@huaweicloud.com> References: <20221028165423.386151-1-roberto.sassu@huaweicloud.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-CM-TRANSID: LxC2BwBX9XFOCVxj63ccAA--.45991S3 X-Coremail-Antispam: 1UD129KBjvJXoW3Xw4rCr15ur1rGFWxAw4DXFb_yoW3Cw4Upr 4xJFyYkrsYvrZIqa4Iyan5Zws5AF1Fga1DKr1DGryIkrZ2vrykJw1UCryjqr9xWryUGrsa gr4qvF4Yg347ZaDanT9S1TB71UUUUUDqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUPqb4IE77IF4wAFF20E14v26rWj6s0DM7CY07I20VC2zVCF04k2 6cxKx2IYs7xG6rWj6s0DM7CIcVAFz4kK6r1j6r18M28IrcIa0xkI8VA2jI8067AKxVWUGw A2048vs2IY020Ec7CjxVAFwI0_Xr0E3s1l8cAvFVAK0II2c7xJM28CjxkF64kEwVA0rcxS w2x7M28EF7xvwVC0I7IYx2IY67AKxVWUJVWUCwA2z4x0Y4vE2Ix0cI8IcVCY1x0267AKxV W8Jr0_Cr1UM28EF7xvwVC2z280aVAFwI0_Gr0_Cr1l84ACjcxK6I8E87Iv6xkF7I0E14v2 6r4UJVWxJr1le2I262IYc4CY6c8Ij28IcVAaY2xG8wAqx4xG64xvF2IEw4CE5I8CrVC2j2 WlYx0E2Ix0cI8IcVAFwI0_JrI_JrylYx0Ex4A2jsIE14v26r1j6r4UMcvjeVCFs4IE7xkE bVWUJVW8JwACjcxG0xvY0x0EwIxGrwACI402YVCY1x02628vn2kIc2xKxwCY1x0262kKe7 AKxVWrXVW3AwCF04k20xvY0x0EwIxGrwCFx2IqxVCFs4IE7xkEbVWUJVW8JwC20s026c02 F40E14v26r1j6r18MI8I3I0E7480Y4vE14v26r106r1rMI8E67AF67kF1VAFwI0_GFv_Wr ylIxkGc2Ij64vIr41lIxAIcVC0I7IYx2IY67AKxVWUJVWUCwCI42IY6xIIjxv20xvEc7Cj xVAFwI0_Gr1j6F4UJwCI42IY6xAIw20EY4v20xvaj40_Jr0_JF4lIxAIcVC2z280aVAFwI 0_Jr0_Gr1lIxAIcVC2z280aVCY1x0267AKxVW8Jr0_Cr1UYxBIdaVFxhVjvjDU0xZFpf9x 07j-kuxUUUUU= X-CM-SenderInfo: purev21wro2thvvxqx5xdzvxpfor3voofrz/1tbiAQATBF1jj4TMWgAAs1 X-CFilter-Loop: Reflected X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_NONE, SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1747951490746917606?= X-GMAIL-MSGID: =?utf-8?q?1747951490746917606?= |
Series |
[RESEND,RFC,1/3] lsm: Clarify documentation of vm_enough_memory hook
|
|
Commit Message
Roberto Sassu
Oct. 28, 2022, 4:54 p.m. UTC
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 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(-)
Comments
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. > 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? > 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(¬_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(¬_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 >
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(¬_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(¬_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 > >
On Fri, Nov 4, 2022 at 8:29 AM Roberto Sassu <roberto.sassu@huaweicloud.com> wrote: > > 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? Before adding infra to all hooks, let's analyze all hooks first. I thought the number of exceptions is very small. 99% of hooks will be fine with IS_ERR. If so, adding an extra flag to every hook will cause too much churn.
On Fri, 2022-11-04 at 17:42 -0700, Alexei Starovoitov wrote: > On Fri, Nov 4, 2022 at 8:29 AM Roberto Sassu > <roberto.sassu@huaweicloud.com> wrote: > > 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? > > Before adding infra to all hooks, let's analyze all hooks first. > I thought the number of exceptions is very small. > 99% of hooks will be fine with IS_ERR. > If so, adding an extra flag to every hook will cause too much churn. If I counted them correctly, there are 12 hooks which can return a positive value. Among them, 6 can return only 1. 3 should not return a negative value. A reason for making this change in the LSM infrastructure would be that the return values provided there would be the official reference for anyone dealing with LSM hooks (e.g. redefining the LSM_HOOK macro). Another reason would be that for new hooks, the developer introducing them would have to provide the information. BPF LSM would use that automatically (otherwise it might get out of sync). The idea would be to use BTF_ID_FLAGS() with the flags coming from lsm_hook_defs.h, and to check if a flag is set depending on the interval of the return value provided by the eBPF program. Roberto
On Mon, Nov 7, 2022 at 4:33 AM Roberto Sassu <roberto.sassu@huaweicloud.com> wrote: > > On Fri, 2022-11-04 at 17:42 -0700, Alexei Starovoitov wrote: > > On Fri, Nov 4, 2022 at 8:29 AM Roberto Sassu > > <roberto.sassu@huaweicloud.com> wrote: > > > 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? > > > > Before adding infra to all hooks, let's analyze all hooks first. > > I thought the number of exceptions is very small. > > 99% of hooks will be fine with IS_ERR. > > If so, adding an extra flag to every hook will cause too much churn. > > If I counted them correctly, there are 12 hooks which can return a > positive value. Among them, 6 can return only 1. 3 should not return a > negative value. > > A reason for making this change in the LSM infrastructure would be that > the return values provided there would be the official reference for > anyone dealing with LSM hooks (e.g. redefining the LSM_HOOK macro). > > Another reason would be that for new hooks, the developer introducing > them would have to provide the information. BPF LSM would use that > automatically (otherwise it might get out of sync). I'd prefer these 12 hooks to get converted to IS_ERR instead. Especially those that can only return 1... why aren't they void? > The idea would be to use BTF_ID_FLAGS() with the flags coming from > lsm_hook_defs.h, and to check if a flag is set depending on the > interval of the return value provided by the eBPF program. BTF_ID_FLAGS is not appropriate for this.
On Mon, 2022-11-07 at 08:00 -0800, Alexei Starovoitov wrote: > On Mon, Nov 7, 2022 at 4:33 AM Roberto Sassu > <roberto.sassu@huaweicloud.com> wrote: > > On Fri, 2022-11-04 at 17:42 -0700, Alexei Starovoitov wrote: > > > On Fri, Nov 4, 2022 at 8:29 AM Roberto Sassu > > > <roberto.sassu@huaweicloud.com> wrote: > > > > 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? > > > > > > Before adding infra to all hooks, let's analyze all hooks first. > > > I thought the number of exceptions is very small. > > > 99% of hooks will be fine with IS_ERR. > > > If so, adding an extra flag to every hook will cause too much churn. > > > > If I counted them correctly, there are 12 hooks which can return a > > positive value. Among them, 6 can return only 1. 3 should not return a > > negative value. > > > > A reason for making this change in the LSM infrastructure would be that > > the return values provided there would be the official reference for > > anyone dealing with LSM hooks (e.g. redefining the LSM_HOOK macro). > > > > Another reason would be that for new hooks, the developer introducing > > them would have to provide the information. BPF LSM would use that > > automatically (otherwise it might get out of sync). > > I'd prefer these 12 hooks to get converted to IS_ERR instead. > Especially those that can only return 1... why aren't they void? Sorry, I meant can only return 1 as positive value (but can return 0 or a negative value). Not sure it is a good idea to change the conventions. > > The idea would be to use BTF_ID_FLAGS() with the flags coming from > > lsm_hook_defs.h, and to check if a flag is set depending on the > > interval of the return value provided by the eBPF program. > > BTF_ID_FLAGS is not appropriate for this. Uhm, why not? If you store the flags in the BTF ID set, the implementation would be something like this. Assuming that a hook definition becomes: LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, path_unlink, const struct path *dir, struct dentry *dentry) In bpf_lsm.c, we would have: #include <linux/lsm_hook_defs.h> #undef LSM_HOOK #define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \ BTF_ID_FLAGS(func, bpf_lsm_##NAME, RET_FLAGS) BTF_SET_START(bpf_lsm_hooks) #include <linux/lsm_hook_defs.h> #undef LSM_HOOK BTF_SET_END(bpf_lsm_hooks) bool bpf_lsm_ret_value_allowed(u32 btf_id, ...) { u32 *flags = btf_id_set8_contains(&bpf_lsm_hooks, btf_id); if (lsm_ret < 0 && !(*flags & LSM_RET_NEG)) return false; ... return true; } Thanks Roberto
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); 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(¬_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(¬_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) {