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:     kpsingh@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, nicolas.bouchinet@clip-os.org
Subject: [RFC][PATCH] bpf: Check xattr name/value pair from bpf_lsm_inode_init_security()
Date:   Fri, 21 Oct 2022 18:46:26 +0200
Message-Id: <20221021164626.3729012-1-roberto.sassu@huaweicloud.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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

BPF LSM allows security modules to directly attach to the security hooks,
with the potential of not meeting the kernel expectation.

This is the case for the inode_init_security hook, for which the kernel
expects that name and value are set if the hook implementation returns
zero.

Consequently, not meeting the kernel expectation can cause the kernel to
crash. One example is evm_protected_xattr_common() which expects the
req_xattr_name parameter to be always not NULL.

Introduce a level of indirection in BPF LSM, for the inode_init_security
hook, to check the validity of the name and value set by security modules.

Encapsulate bpf_lsm_inode_init_security(), the existing attachment point,
with bpf_inode_init_security(), the new function. After the attachment
point is called, return -EOPNOTSUPP if the xattr name is not set, -ENOMEM
if the xattr value is not set.

As the name still cannot be set, rely on future patches to the eBPF
verifier or introducing new kfuncs/helpers to ensure its correctness.

Finally, as proposed by Nicolas, update the LSM hook documentation for the
inode_init_security hook, to reflect the current behavior (only the xattr
value is allocated).

Cc: stable@vger.kernel.org
Fixes: 520b7aa00d8cd ("bpf: lsm: Initialize the BPF LSM hooks")
Reported-by: Nicolas Bouchinet <nicolas.bouchinet@clip-os.org>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 include/linux/lsm_hooks.h |  4 ++--
 security/bpf/hooks.c      | 25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 4ec80b96c22e..f44d45f4737f 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -229,8 +229,8 @@
  *	This hook is called by the fs code as part of the inode creation
  *	transaction and provides for atomic labeling of the inode, unlike
  *	the post_create/mkdir/... hooks called by the VFS.  The hook function
- *	is expected to allocate the name and value via kmalloc, with the caller
- *	being responsible for calling kfree after using them.
+ *	is expected to allocate the value via kmalloc, with the caller
+ *	being responsible for calling kfree after using it.
  *	If the security module does not use security attributes or does
  *	not wish to put a security attribute on this particular inode,
  *	then it should return -EOPNOTSUPP to skip this processing.
diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
index e5971fa74fd7..492c07ba6722 100644
--- a/security/bpf/hooks.c
+++ b/security/bpf/hooks.c
@@ -6,11 +6,36 @@
 #include <linux/lsm_hooks.h>
 #include <linux/bpf_lsm.h>
 
+static int bpf_inode_init_security(struct inode *inode, struct inode *dir,
+				   const struct qstr *qstr, const char **name,
+				   void **value, size_t *len)
+{
+	int ret;
+
+	ret = bpf_lsm_inode_init_security(inode, dir, qstr, name, value, len);
+	if (ret)
+		return ret;
+
+	/*
+	 * As the name cannot be set by the eBPF programs directly, eBPF will
+	 * be responsible for its correctness through the verifier or
+	 * appropriate kfuncs/helpers.
+	 */
+	if (name && !*name)
+		return -EOPNOTSUPP;
+
+	if (value && !*value)
+		return -ENOMEM;
+
+	return 0;
+}
+
 static struct security_hook_list bpf_lsm_hooks[] __lsm_ro_after_init = {
 	#define LSM_HOOK(RET, DEFAULT, NAME, ...) \
 	LSM_HOOK_INIT(NAME, bpf_lsm_##NAME),
 	#include <linux/lsm_hook_defs.h>
 	#undef LSM_HOOK
+	LSM_HOOK_INIT(inode_init_security, bpf_inode_init_security),
 	LSM_HOOK_INIT(inode_free_security, bpf_inode_storage_free),
 	LSM_HOOK_INIT(task_free, bpf_task_storage_free),
 };
-- 
2.25.1