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;
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, revest@chromium.org,
        jackmanb@chromium.org, paul@paul-moore.com, jmorris@namei.org,
        serge@hallyn.com
Cc:     bpf@vger.kernel.org, linux-security-module@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Roberto Sassu <roberto.sassu@huawei.com>
Subject: [RFC][PATCH 4/4] security: Enforce limitations on return values from LSMs
Date:   Tue, 15 Nov 2022 18:56:52 +0100
Message-Id: <20221115175652.3836811-5-roberto.sassu@huaweicloud.com>
In-Reply-To: <20221115175652.3836811-1-roberto.sassu@huaweicloud.com>
References: <20221115175652.3836811-1-roberto.sassu@huaweicloud.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

From: Roberto Sassu <roberto.sassu@huawei.com>

LSMs should not be able to return arbitrary return values, as the callers
of the LSM infrastructure might not be ready to handle unexpected values
(e.g. positive values that are first converted to a pointer with ERR_PTR,
and then evaluated with IS_ERR()).

Modify call_int_hook() to call is_ret_value_allowed(), so that the return
value from each LSM for a given hook is checked. If for the interval the
return value falls into the corresponding flag is not set, change the
return value to the default value, just for the current LSM.

A misbehaving LSM would not have impact on the decision of other LSMs, as
the loop terminates whenever the return value is not zero.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/security.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/security/security.c b/security/security.c
index 4041d24e3283..cd417a8a0e65 100644
--- a/security/security.c
+++ b/security/security.c
@@ -716,6 +716,35 @@ static int lsm_superblock_alloc(struct super_block *sb)
 #include <linux/lsm_hook_defs.h>
 #undef LSM_HOOK
 
+/*
+ * The return value flags of the LSM hook are defined in linux/lsm_hook_defs.h
+ * and can be accessed with:
+ *
+ *	LSM_RET_FLAGS(<hook_name>)
+ *
+ * The macros below define static constants for the return value flags of each
+ * LSM hook.
+ */
+#define LSM_RET_FLAGS(NAME) (NAME##_ret_flags)
+#define DECLARE_LSM_RET_FLAGS(RET_FLAGS, NAME) \
+	static const u32 __maybe_unused LSM_RET_FLAGS(NAME) = (RET_FLAGS);
+#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \
+	DECLARE_LSM_RET_FLAGS(RET_FLAGS, NAME)
+
+#include <linux/lsm_hook_defs.h>
+#undef LSM_HOOK
+
+static bool is_ret_value_allowed(int ret, u32 ret_flags)
+{
+	if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) ||
+	    (ret == 0 && !(ret_flags & LSM_RET_ZERO)) ||
+	    (ret == 1 && !(ret_flags & LSM_RET_ONE)) ||
+	    (ret > 1 && !(ret_flags & LSM_RET_GT_ONE)))
+		return false;
+
+	return true;
+}
+
 /*
  * Hook list operation macros.
  *
@@ -741,6 +770,11 @@ static int lsm_superblock_alloc(struct super_block *sb)
 								\
 		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
 			RC = P->hook.FUNC(__VA_ARGS__);		\
+			if (!is_ret_value_allowed(RC, LSM_RET_FLAGS(FUNC))) { \
+				WARN_ONCE(1, "Illegal ret %d for " #FUNC " from %s, forcing %d\n", \
+					  RC, P->lsm, LSM_RET_DEFAULT(FUNC)); \
+				RC = LSM_RET_DEFAULT(FUNC);	\
+			}					\
 			if (RC != 0)				\
 				break;				\
 		}						\
-- 
2.25.1