From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Roberto Sassu <roberto.sassu@huaweicloud.com>
Cc: KP Singh <kpsingh@kernel.org>,
Florent Revest <revest@chromium.org>,
Brendan Jackman <jackmanb@chromium.org>,
Paul Moore <paul@paul-moore.com>,
James Morris <jmorris@namei.org>,
"Serge E . Hallyn" <serge@hallyn.com>, bpf <bpf@vger.kernel.org>,
LSM List <linux-security-module@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
nicolas.bouchinet@clip-os.org
Subject: Re: [RFC][PATCH] bpf: Check xattr name/value pair from bpf_lsm_inode_init_security()
Date: Sun, 23 Oct 2022 16:36:22 -0700 [thread overview]
Message-ID: <CAADnVQJHDboosqTy5LTHJtJaWJCWn9rv09jmd_sMgeV_OVQjGg@mail.gmail.com> (raw)
In-Reply-To: <20221021164626.3729012-1-roberto.sassu@huaweicloud.com>
On Fri, Oct 21, 2022 at 9:57 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> 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.
Sounds like a bug in evm_protected_xattr_common.
> 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.
Doesn't make sense.
You probably meant security_old_inode_init_security,
because the hook without _old_ doesn't have such args:
int security_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr,
initxattrs initxattrs, void *fs_data);
> 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.
must be an obsolete comment.
> * 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;
bpf cannot write into such pointers.
It won't be able to use kfuncs to kmalloc and write into them either.
None of it makes sense to me.
> +
> + 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
>
next prev parent reply other threads:[~2022-10-23 23:36 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-21 16:46 [RFC][PATCH] bpf: Check xattr name/value pair from bpf_lsm_inode_init_security() Roberto Sassu
2022-10-23 23:36 ` Alexei Starovoitov [this message]
2022-10-24 9:25 ` Roberto Sassu
2022-10-24 15:28 ` Roberto Sassu
2022-10-25 2:13 ` Alexei Starovoitov
2022-10-25 7:43 ` Roberto Sassu
2022-10-25 14:57 ` Casey Schaufler
2022-10-26 6:37 ` Alexei Starovoitov
2022-10-26 8:42 ` Roberto Sassu
2022-10-26 17:14 ` Alexei Starovoitov
2022-10-27 10:39 ` KP Singh
2022-10-27 15:52 ` Casey Schaufler
2022-10-28 8:48 ` Roberto Sassu
2022-10-28 15:01 ` Casey Schaufler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAADnVQJHDboosqTy5LTHJtJaWJCWn9rv09jmd_sMgeV_OVQjGg@mail.gmail.com \
--to=alexei.starovoitov@gmail.com \
--cc=bpf@vger.kernel.org \
--cc=jackmanb@chromium.org \
--cc=jmorris@namei.org \
--cc=kpsingh@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=nicolas.bouchinet@clip-os.org \
--cc=paul@paul-moore.com \
--cc=revest@chromium.org \
--cc=roberto.sassu@huaweicloud.com \
--cc=serge@hallyn.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).