Date:   Fri, 28 Oct 2022 18:54:22 +0200
From:   Roberto Sassu <roberto.sassu@...weicloud.com>
To:     ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
        martin.lau@...ux.dev, song@...nel.org, yhs@...com,
        john.fastabend@...il.com, kpsingh@...nel.org, 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
Cc:     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: [RESEND][RFC][PATCH 2/3] bpf-lsm: Limit values that can be returned by security modules

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