* Re: [PATCH v2] fscrypt: support trusted keys
From: Mimi Zohar @ 2021-08-12 0:54 UTC (permalink / raw)
To: Eric Biggers
Cc: Jarkko Sakkinen, Ahmad Fatoum, Theodore Y. Ts'o, Jaegeuk Kim,
kernel, James Morris, Serge E. Hallyn, James Bottomley,
Sumit Garg, David Howells, linux-fscrypt, linux-crypto,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <YRQF09f8st95yrFZ@gmail.com>
On Wed, 2021-08-11 at 10:16 -0700, Eric Biggers wrote:
> Neither of you actually answered my question, which is whether the support for
> trusted keys in dm-crypt is a mistake. I think you're saying that it is? That
> would imply that fscrypt shouldn't support trusted keys, but rather encrypted
> keys -- which conflicts with Ahmad's patch which is adding support for trusted
> keys. Note that your reasoning for this is not documented at all in the
> trusted-encrypted keys documentation; it needs to be (email threads don't really
> matter), otherwise how would anyone know when/how to use this feature?
True, but all of the trusted-encrypted key examples in the
documentation are "encrypted" type keys, encrypted/decrypted based on a
"trusted" type key. There are no examples of using the "trusted" key
type directly. Before claiming that adding "trusted" key support in
dm-crypt was a mistake, we should ask Ahmad why he felt dm-crypt needed
to directly support "trusted" type keys.
Mimi
^ permalink raw reply
* [RFC PATCH v2 5/9] fs: add anon_inode_getfile_secure() similar to anon_inode_getfd_secure()
From: Paul Moore @ 2021-08-11 20:48 UTC (permalink / raw)
To: linux-security-module, selinux, linux-audit, io-uring,
linux-fsdevel, Kumar Kartikeya Dwivedi, Jens Axboe,
Pavel Begunkov
In-Reply-To: <162871480969.63873.9434591871437326374.stgit@olly>
Extending the secure anonymous inode support to other subsystems
requires that we have a secure anon_inode_getfile() variant in
addition to the existing secure anon_inode_getfd() variant.
Thankfully we can reuse the existing __anon_inode_getfile() function
and just wrap it with the proper arguments.
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
v2:
- no change
v1:
- initial draft
---
fs/anon_inodes.c | 29 +++++++++++++++++++++++++++++
include/linux/anon_inodes.h | 4 ++++
2 files changed, 33 insertions(+)
diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
index a280156138ed..e0c3e33c4177 100644
--- a/fs/anon_inodes.c
+++ b/fs/anon_inodes.c
@@ -148,6 +148,35 @@ struct file *anon_inode_getfile(const char *name,
}
EXPORT_SYMBOL_GPL(anon_inode_getfile);
+/**
+ * anon_inode_getfile_secure - Like anon_inode_getfile(), but creates a new
+ * !S_PRIVATE anon inode rather than reuse the
+ * singleton anon inode and calls the
+ * inode_init_security_anon() LSM hook. This
+ * allows for both the inode to have its own
+ * security context and for the LSM to enforce
+ * policy on the inode's creation.
+ *
+ * @name: [in] name of the "class" of the new file
+ * @fops: [in] file operations for the new file
+ * @priv: [in] private data for the new file (will be file's private_data)
+ * @flags: [in] flags
+ * @context_inode:
+ * [in] the logical relationship with the new inode (optional)
+ *
+ * The LSM may use @context_inode in inode_init_security_anon(), but a
+ * reference to it is not held. Returns the newly created file* or an error
+ * pointer. See the anon_inode_getfile() documentation for more information.
+ */
+struct file *anon_inode_getfile_secure(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode)
+{
+ return __anon_inode_getfile(name, fops, priv, flags,
+ context_inode, true);
+}
+
static int __anon_inode_getfd(const char *name,
const struct file_operations *fops,
void *priv, int flags,
diff --git a/include/linux/anon_inodes.h b/include/linux/anon_inodes.h
index 71881a2b6f78..5deaddbd7927 100644
--- a/include/linux/anon_inodes.h
+++ b/include/linux/anon_inodes.h
@@ -15,6 +15,10 @@ struct inode;
struct file *anon_inode_getfile(const char *name,
const struct file_operations *fops,
void *priv, int flags);
+struct file *anon_inode_getfile_secure(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode);
int anon_inode_getfd(const char *name, const struct file_operations *fops,
void *priv, int flags);
int anon_inode_getfd_secure(const char *name,
^ permalink raw reply related
* [RFC PATCH v2 9/9] Smack: Brutalist io_uring support with debug
From: Paul Moore @ 2021-08-11 20:49 UTC (permalink / raw)
To: linux-security-module, selinux, linux-audit, io-uring,
linux-fsdevel, Kumar Kartikeya Dwivedi, Jens Axboe,
Pavel Begunkov
In-Reply-To: <162871480969.63873.9434591871437326374.stgit@olly>
From: Casey Schaufler <casey@schaufler-ca.com>
Add Smack privilege checks for io_uring. Use CAP_MAC_OVERRIDE
for the override_creds case and CAP_MAC_ADMIN for creating a
polling thread. These choices are based on conjecture regarding
the intent of the surrounding code.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
[PM: make the smack_uring_* funcs static]
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
v2:
- made the smack_uring_* funcs static
v1:
- initial draft
---
security/smack/smack_lsm.c | 64 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 64 insertions(+)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 223a6da0e6dc..7fb094098f38 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4691,6 +4691,66 @@ static int smack_dentry_create_files_as(struct dentry *dentry, int mode,
return 0;
}
+#ifdef CONFIG_IO_URING
+/**
+ * smack_uring_override_creds - Is io_uring cred override allowed?
+ * @new: the target creds
+ *
+ * Check to see if the current task is allowed to override it's credentials
+ * to service an io_uring operation.
+ */
+static int smack_uring_override_creds(const struct cred *new)
+{
+ struct task_smack *tsp = smack_cred(current_cred());
+ struct task_smack *nsp = smack_cred(new);
+
+#if 1
+ if (tsp->smk_task == nsp->smk_task)
+ pr_info("%s: Smack matches %s\n", __func__,
+ tsp->smk_task->smk_known);
+ else
+ pr_info("%s: Smack override check %s to %s\n", __func__,
+ tsp->smk_task->smk_known, nsp->smk_task->smk_known);
+#endif
+ /*
+ * Allow the degenerate case where the new Smack value is
+ * the same as the current Smack value.
+ */
+ if (tsp->smk_task == nsp->smk_task)
+ return 0;
+
+#if 1
+ pr_info("%s: Smack sqpoll %s\n", __func__,
+ smack_privileged_cred(CAP_MAC_OVERRIDE, current_cred()) ?
+ "ok by Smack" : "disallowed (No CAP_MAC_OVERRIDE)");
+#endif
+ if (smack_privileged_cred(CAP_MAC_OVERRIDE, current_cred()))
+ return 0;
+
+ return -EPERM;
+}
+
+/**
+ * smack_uring_sqpoll - check if a io_uring polling thread can be created
+ *
+ * Check to see if the current task is allowed to create a new io_uring
+ * kernel polling thread.
+ */
+static int smack_uring_sqpoll(void)
+{
+#if 1
+ pr_info("%s: Smack new ring %s\n", __func__,
+ smack_privileged_cred(CAP_MAC_ADMIN, current_cred()) ?
+ "ok by Smack" : "disallowed (No CAP_MAC_ADMIN)");
+#endif
+ if (smack_privileged_cred(CAP_MAC_ADMIN, current_cred()))
+ return 0;
+
+ return -EPERM;
+}
+
+#endif /* CONFIG_IO_URING */
+
struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
.lbs_cred = sizeof(struct task_smack),
.lbs_file = sizeof(struct smack_known *),
@@ -4843,6 +4903,10 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(inode_copy_up, smack_inode_copy_up),
LSM_HOOK_INIT(inode_copy_up_xattr, smack_inode_copy_up_xattr),
LSM_HOOK_INIT(dentry_create_files_as, smack_dentry_create_files_as),
+#ifdef CONFIG_IO_URING
+ LSM_HOOK_INIT(uring_override_creds, smack_uring_override_creds),
+ LSM_HOOK_INIT(uring_sqpoll, smack_uring_sqpoll),
+#endif
};
^ permalink raw reply related
* [RFC PATCH v2 8/9] selinux: add support for the io_uring access controls
From: Paul Moore @ 2021-08-11 20:49 UTC (permalink / raw)
To: linux-security-module, selinux, linux-audit, io-uring,
linux-fsdevel, Kumar Kartikeya Dwivedi, Jens Axboe,
Pavel Begunkov
In-Reply-To: <162871480969.63873.9434591871437326374.stgit@olly>
WARNING - This is a work in progress, this patch, including the
description, may be incomplete or even incorrect. You have been
warned.
This patch implements two new io_uring access controls, specifically
support for controlling the io_uring "personalities" and
IORING_SETUP_SQPOLL. Controlling the sharing of io_urings themselves
is handled via the normal file/inode labeling and sharing mechanisms.
The io_uring { override_creds } permission restricts which domains
the subject domain can use to override it's own credentials.
Granting a domain the io_uring { override_creds } permission allows
it to impersonate another domain in io_uring operations.
The io_uring { sqpoll } permission restricts which domains can create
asynchronous io_uring polling threads. This is important from a
security perspective as operations queued by this asynchronous thread
inherit the credentials of the thread creator by default; if an
io_uring is shared across process/domain boundaries this could result
in one domain impersonating another. Controlling the creation of
sqpoll threads, and the sharing of io_urings across processes, allow
policy authors to restrict the ability of one domain to impersonate
another via io_uring.
As a quick summary, this patch adds a new object class with two
permissions:
io_uring { override_creds sqpoll }
These permissions can be seen in the two simple policy statements
below:
allow domA_t domB_t : io_uring { override_creds };
allow domA_t self : io_uring { sqpoll };
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
v2:
- made the selinux_uring_* funcs static
- removed the debugging code
v1:
- initial draft
---
security/selinux/hooks.c | 34 ++++++++++++++++++++++++++++++++++
security/selinux/include/classmap.h | 2 ++
2 files changed, 36 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index b0032c42333e..1fb0c76deff2 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -7105,6 +7105,35 @@ static int selinux_perf_event_write(struct perf_event *event)
}
#endif
+#ifdef CONFIG_IO_URING
+/**
+ * selinux_uring_override_creds - check the requested cred override
+ * @new: the target creds
+ *
+ * Check to see if the current task is allowed to override it's credentials
+ * to service an io_uring operation.
+ */
+static int selinux_uring_override_creds(const struct cred *new)
+{
+ return avc_has_perm(&selinux_state, current_sid(), cred_sid(new),
+ SECCLASS_IO_URING, IO_URING__OVERRIDE_CREDS, NULL);
+}
+
+/**
+ * selinux_uring_sqpoll - check if a io_uring polling thread can be created
+ *
+ * Check to see if the current task is allowed to create a new io_uring
+ * kernel polling thread.
+ */
+static int selinux_uring_sqpoll(void)
+{
+ int sid = current_sid();
+
+ return avc_has_perm(&selinux_state, sid, sid,
+ SECCLASS_IO_URING, IO_URING__SQPOLL, NULL);
+}
+#endif /* CONFIG_IO_URING */
+
/*
* IMPORTANT NOTE: When adding new hooks, please be careful to keep this order:
* 1. any hooks that don't belong to (2.) or (3.) below,
@@ -7343,6 +7372,11 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(perf_event_write, selinux_perf_event_write),
#endif
+#ifdef CONFIG_IO_URING
+ LSM_HOOK_INIT(uring_override_creds, selinux_uring_override_creds),
+ LSM_HOOK_INIT(uring_sqpoll, selinux_uring_sqpoll),
+#endif
+
LSM_HOOK_INIT(locked_down, selinux_lockdown),
/*
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 62d19bccf3de..3314ad72279d 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -252,6 +252,8 @@ struct security_class_mapping secclass_map[] = {
{ "integrity", "confidentiality", NULL } },
{ "anon_inode",
{ COMMON_FILE_PERMS, NULL } },
+ { "io_uring",
+ { "override_creds", "sqpoll", NULL } },
{ NULL }
};
^ permalink raw reply related
* [RFC PATCH v2 7/9] lsm,io_uring: add LSM hooks to io_uring
From: Paul Moore @ 2021-08-11 20:48 UTC (permalink / raw)
To: linux-security-module, selinux, linux-audit, io-uring,
linux-fsdevel, Kumar Kartikeya Dwivedi, Jens Axboe,
Pavel Begunkov
In-Reply-To: <162871480969.63873.9434591871437326374.stgit@olly>
WARNING - This is a work in progress, this patch, including the
description, may be incomplete or even incorrect. You have been
warned.
A full expalantion of io_uring is beyond the scope of this commit
description, but in summary it is an asynchronous I/O mechanism
which allows for I/O requests and the resulting data to be queued
in memory mapped "rings" which are shared between the kernel and
userspace. Optionally, io_uring offers the ability for applications
to spawn kernel threads to dequeue I/O requests from the ring and
submit the requests in the kernel, helping to minimize the syscall
overhead. Rings are accessed in userspace by memory mapping a file
descriptor provided by the io_uring_setup(2), and can be shared
between applications as one might do with any open file descriptor.
Finally, process credentials can be registered with a given ring
and any process with access to that ring can submit I/O requests
using any of the registered credentials.
While the io_uring functionality is widely recognized as offering a
vastly improved, and high performing asynchronous I/O mechanism, its
ability to allow processes to submit I/O requests with credentials
other than its own presents a challenge to LSMs. When a process
creates a new io_uring ring the ring's credentials are inhertied
from the calling process; if this ring is shared with another
process operating with different credentials there is the potential
to bypass the LSMs security policy. Similarly, registering
credentials with a given ring allows any process with access to that
ring to submit I/O requests with those credentials.
In an effort to allow LSMs to apply security policy to io_uring I/O
operations, this patch adds two new LSM hooks. These hooks, in
conjunction with the LSM anonymous inode support previously
submitted, allow an LSM to apply access control policy to the
sharing of io_uring rings as well as any io_uring credential changes
requested by a process.
The new LSM hooks are described below:
* int security_uring_override_creds(cred)
Controls if the current task, executing an io_uring operation,
is allowed to override it's credentials with @cred. In cases
where the current task is a user application, the current
credentials will be those of the user application. In cases
where the current task is a kernel thread servicing io_uring
requests the current credentials will be those of the io_uring
ring (inherited from the process that created the ring).
* int security_uring_sqpoll(void)
Controls if the current task is allowed to create an io_uring
polling thread (IORING_SETUP_SQPOLL). Without a SQPOLL thread
in the kernel processes must submit I/O requests via
io_uring_enter(2) which allows us to compare any requested
credential changes against the application making the request.
With a SQPOLL thread, we can no longer compare requested
credential changes against the application making the request,
the comparison is made against the ring's credentials.
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
v2:
- no change
v1:
- initial draft
---
fs/io_uring.c | 10 ++++++++++
include/linux/lsm_hook_defs.h | 5 +++++
include/linux/lsm_hooks.h | 13 +++++++++++++
include/linux/security.h | 16 ++++++++++++++++
security/security.c | 12 ++++++++++++
5 files changed, 56 insertions(+)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index ea396f5fe735..78df778d329b 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -79,6 +79,7 @@
#include <linux/pagemap.h>
#include <linux/io_uring.h>
#include <linux/audit.h>
+#include <linux/security.h>
#define CREATE_TRACE_POINTS
#include <trace/events/io_uring.h>
@@ -6617,6 +6618,11 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
if (!req->creds)
return -EINVAL;
get_cred(req->creds);
+ ret = security_uring_override_creds(req->creds);
+ if (ret) {
+ put_cred(req->creds);
+ return ret;
+ }
req->flags |= REQ_F_CREDS;
}
state = &ctx->submit_state;
@@ -8080,6 +8086,10 @@ static int io_sq_offload_create(struct io_ring_ctx *ctx,
struct io_sq_data *sqd;
bool attached;
+ ret = security_uring_sqpoll();
+ if (ret)
+ return ret;
+
sqd = io_get_sq_data(p, &attached);
if (IS_ERR(sqd)) {
ret = PTR_ERR(sqd);
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 2adeea44c0d5..b3c525353769 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -402,3 +402,8 @@ LSM_HOOK(void, LSM_RET_VOID, perf_event_free, struct perf_event *event)
LSM_HOOK(int, 0, perf_event_read, struct perf_event *event)
LSM_HOOK(int, 0, perf_event_write, struct perf_event *event)
#endif /* CONFIG_PERF_EVENTS */
+
+#ifdef CONFIG_IO_URING
+LSM_HOOK(int, 0, uring_override_creds, const struct cred *new)
+LSM_HOOK(int, 0, uring_sqpoll, void)
+#endif /* CONFIG_IO_URING */
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 5c4c5c0602cb..0eb0ae95c4c4 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1557,6 +1557,19 @@
* Read perf_event security info if allowed.
* @perf_event_write:
* Write perf_event security info if allowed.
+ *
+ * Security hooks for io_uring
+ *
+ * @uring_override_creds:
+ * Check if the current task, executing an io_uring operation, is allowed
+ * to override it's credentials with @new.
+ *
+ * @new: the new creds to use
+ *
+ * @uring_sqpoll:
+ * Check whether the current task is allowed to spawn a io_uring polling
+ * thread (IORING_SETUP_SQPOLL).
+ *
*/
union security_list_options {
#define LSM_HOOK(RET, DEFAULT, NAME, ...) RET (*NAME)(__VA_ARGS__);
diff --git a/include/linux/security.h b/include/linux/security.h
index 24eda04221e9..3928d80b6f76 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -2037,4 +2037,20 @@ static inline int security_perf_event_write(struct perf_event *event)
#endif /* CONFIG_SECURITY */
#endif /* CONFIG_PERF_EVENTS */
+#ifdef CONFIG_IO_URING
+#ifdef CONFIG_SECURITY
+extern int security_uring_override_creds(const struct cred *new);
+extern int security_uring_sqpoll(void);
+#else
+static inline int security_uring_override_creds(const struct cred *new)
+{
+ return 0;
+}
+static inline int security_uring_sqpoll(void)
+{
+ return 0;
+}
+#endif /* CONFIG_SECURITY */
+#endif /* CONFIG_IO_URING */
+
#endif /* ! __LINUX_SECURITY_H */
diff --git a/security/security.c b/security/security.c
index 09533cbb7221..a54544e61bd1 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2624,3 +2624,15 @@ int security_perf_event_write(struct perf_event *event)
return call_int_hook(perf_event_write, 0, event);
}
#endif /* CONFIG_PERF_EVENTS */
+
+#ifdef CONFIG_IO_URING
+int security_uring_override_creds(const struct cred *new)
+{
+ return call_int_hook(uring_override_creds, 0, new);
+}
+
+int security_uring_sqpoll(void)
+{
+ return call_int_hook(uring_sqpoll, 0);
+}
+#endif /* CONFIG_IO_URING */
^ permalink raw reply related
* [RFC PATCH v2 6/9] io_uring: convert io_uring to the secure anon inode interface
From: Paul Moore @ 2021-08-11 20:48 UTC (permalink / raw)
To: linux-security-module, selinux, linux-audit, io-uring,
linux-fsdevel, Kumar Kartikeya Dwivedi, Jens Axboe,
Pavel Begunkov
In-Reply-To: <162871480969.63873.9434591871437326374.stgit@olly>
Converting io_uring's anonymous inode to the secure anon inode API
enables LSMs to enforce policy on the io_uring anonymous inodes if
they chose to do so. This is an important first step towards
providing the necessary mechanisms so that LSMs can apply security
policy to io_uring operations.
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
v2:
- no change
v1:
- initial draft
---
fs/io_uring.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index b407a6ea1779..ea396f5fe735 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -9662,8 +9662,8 @@ static struct file *io_uring_get_file(struct io_ring_ctx *ctx)
return ERR_PTR(ret);
#endif
- file = anon_inode_getfile("[io_uring]", &io_uring_fops, ctx,
- O_RDWR | O_CLOEXEC);
+ file = anon_inode_getfile_secure("[io_uring]", &io_uring_fops, ctx,
+ O_RDWR | O_CLOEXEC, NULL);
#if defined(CONFIG_UNIX)
if (IS_ERR(file)) {
sock_release(ctx->ring_sock);
^ permalink raw reply related
* [RFC PATCH v2 4/9] audit: add filtering for io_uring records
From: Paul Moore @ 2021-08-11 20:48 UTC (permalink / raw)
To: linux-security-module, selinux, linux-audit, io-uring,
linux-fsdevel, Kumar Kartikeya Dwivedi, Jens Axboe,
Pavel Begunkov
In-Reply-To: <162871480969.63873.9434591871437326374.stgit@olly>
WARNING - This is a work in progress and should not be merged
anywhere important. It is almost surely not complete, and while it
probably compiles it likely hasn't been booted and will do terrible
things. You have been warned.
This patch adds basic audit io_uring filtering, using as much of the
existing audit filtering infrastructure as possible. In order to do
this we reuse the audit filter rule's syscall mask for the io_uring
operation and we create a new filter for io_uring operations as
AUDIT_FILTER_URING_EXIT/audit_filter_list[7].
<TODO - provide some additional guidance for the userspace tools>
Thanks to Richard Guy Briggs for his review and feedback.
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
v2:
- incorporate feedback from Richard
v1:
- initial draft
---
include/uapi/linux/audit.h | 3 +-
kernel/audit_tree.c | 3 +-
kernel/audit_watch.c | 3 +-
kernel/auditfilter.c | 15 ++++++++--
kernel/auditsc.c | 65 ++++++++++++++++++++++++++++++++++----------
5 files changed, 68 insertions(+), 21 deletions(-)
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index a1997697c8b1..ecf1edd2affa 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -167,8 +167,9 @@
#define AUDIT_FILTER_EXCLUDE 0x05 /* Apply rule before record creation */
#define AUDIT_FILTER_TYPE AUDIT_FILTER_EXCLUDE /* obsolete misleading naming */
#define AUDIT_FILTER_FS 0x06 /* Apply rule at __audit_inode_child */
+#define AUDIT_FILTER_URING_EXIT 0x07 /* Apply rule at io_uring op exit */
-#define AUDIT_NR_FILTERS 7
+#define AUDIT_NR_FILTERS 8
#define AUDIT_FILTER_PREPEND 0x10 /* Prepend to front of list */
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index b2be4e978ba3..31bf34844546 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -726,7 +726,8 @@ int audit_make_tree(struct audit_krule *rule, char *pathname, u32 op)
{
if (pathname[0] != '/' ||
- rule->listnr != AUDIT_FILTER_EXIT ||
+ (rule->listnr != AUDIT_FILTER_EXIT &&
+ rule->listnr != AUDIT_FILTER_URING_EXIT) ||
op != Audit_equal ||
rule->inode_f || rule->watch || rule->tree)
return -EINVAL;
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 2acf7ca49154..698b62b4a2ec 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -183,7 +183,8 @@ int audit_to_watch(struct audit_krule *krule, char *path, int len, u32 op)
return -EOPNOTSUPP;
if (path[0] != '/' || path[len-1] == '/' ||
- krule->listnr != AUDIT_FILTER_EXIT ||
+ (krule->listnr != AUDIT_FILTER_EXIT &&
+ krule->listnr != AUDIT_FILTER_URING_EXIT) ||
op != Audit_equal ||
krule->inode_f || krule->watch || krule->tree)
return -EINVAL;
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index db2c6b59dfc3..d75acb014ccd 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -44,7 +44,8 @@ struct list_head audit_filter_list[AUDIT_NR_FILTERS] = {
LIST_HEAD_INIT(audit_filter_list[4]),
LIST_HEAD_INIT(audit_filter_list[5]),
LIST_HEAD_INIT(audit_filter_list[6]),
-#if AUDIT_NR_FILTERS != 7
+ LIST_HEAD_INIT(audit_filter_list[7]),
+#if AUDIT_NR_FILTERS != 8
#error Fix audit_filter_list initialiser
#endif
};
@@ -56,6 +57,7 @@ static struct list_head audit_rules_list[AUDIT_NR_FILTERS] = {
LIST_HEAD_INIT(audit_rules_list[4]),
LIST_HEAD_INIT(audit_rules_list[5]),
LIST_HEAD_INIT(audit_rules_list[6]),
+ LIST_HEAD_INIT(audit_rules_list[7]),
};
DEFINE_MUTEX(audit_filter_mutex);
@@ -151,7 +153,8 @@ char *audit_unpack_string(void **bufp, size_t *remain, size_t len)
static inline int audit_to_inode(struct audit_krule *krule,
struct audit_field *f)
{
- if (krule->listnr != AUDIT_FILTER_EXIT ||
+ if ((krule->listnr != AUDIT_FILTER_EXIT &&
+ krule->listnr != AUDIT_FILTER_URING_EXIT) ||
krule->inode_f || krule->watch || krule->tree ||
(f->op != Audit_equal && f->op != Audit_not_equal))
return -EINVAL;
@@ -248,6 +251,7 @@ static inline struct audit_entry *audit_to_entry_common(struct audit_rule_data *
pr_err("AUDIT_FILTER_ENTRY is deprecated\n");
goto exit_err;
case AUDIT_FILTER_EXIT:
+ case AUDIT_FILTER_URING_EXIT:
case AUDIT_FILTER_TASK:
#endif
case AUDIT_FILTER_USER:
@@ -332,6 +336,10 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
if (entry->rule.listnr != AUDIT_FILTER_FS)
return -EINVAL;
break;
+ case AUDIT_PERM:
+ if (entry->rule.listnr == AUDIT_FILTER_URING_EXIT)
+ return -EINVAL;
+ break;
}
switch (entry->rule.listnr) {
@@ -980,7 +988,8 @@ static inline int audit_add_rule(struct audit_entry *entry)
}
entry->rule.prio = ~0ULL;
- if (entry->rule.listnr == AUDIT_FILTER_EXIT) {
+ if (entry->rule.listnr == AUDIT_FILTER_EXIT ||
+ entry->rule.listnr == AUDIT_FILTER_URING_EXIT) {
if (entry->rule.flags & AUDIT_FILTER_PREPEND)
entry->rule.prio = ++prio_high;
else
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 928f1dd12460..f4a2ca496cac 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -805,6 +805,35 @@ static int audit_in_mask(const struct audit_krule *rule, unsigned long val)
return rule->mask[word] & bit;
}
+/**
+ * audit_filter_uring - apply filters to an io_uring operation
+ * @tsk: associated task
+ * @ctx: audit context
+ */
+static void audit_filter_uring(struct task_struct *tsk,
+ struct audit_context *ctx)
+{
+ struct audit_entry *e;
+ enum audit_state state;
+
+ if (auditd_test_task(tsk))
+ return;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_URING_EXIT],
+ list) {
+ if (audit_in_mask(&e->rule, ctx->uring_op) &&
+ audit_filter_rules(tsk, &e->rule, ctx, NULL, &state,
+ false)) {
+ rcu_read_unlock();
+ ctx->current_state = state;
+ return;
+ }
+ }
+ rcu_read_unlock();
+ return;
+}
+
/* At syscall exit time, this filter is called if the audit_state is
* not low enough that auditing cannot take place, but is also not
* high enough that we already know we have to write an audit record
@@ -1765,7 +1794,7 @@ static void audit_log_exit(void)
* __audit_free - free a per-task audit context
* @tsk: task whose audit context block to free
*
- * Called from copy_process and do_exit
+ * Called from copy_process, do_exit, and the io_uring code
*/
void __audit_free(struct task_struct *tsk)
{
@@ -1783,15 +1812,21 @@ void __audit_free(struct task_struct *tsk)
* random task_struct that doesn't doesn't have any meaningful data we
* need to log via audit_log_exit().
*/
- if (tsk == current && !context->dummy &&
- context->context == AUDIT_CTX_SYSCALL) {
+ if (tsk == current && !context->dummy) {
context->return_valid = AUDITSC_INVALID;
context->return_code = 0;
-
- audit_filter_syscall(tsk, context);
- audit_filter_inodes(tsk, context);
- if (context->current_state == AUDIT_STATE_RECORD)
- audit_log_exit();
+ if (context->context == AUDIT_CTX_SYSCALL) {
+ audit_filter_syscall(tsk, context);
+ audit_filter_inodes(tsk, context);
+ if (context->current_state == AUDIT_STATE_RECORD)
+ audit_log_exit();
+ } else if (context->context == AUDIT_CTX_URING) {
+ /* TODO: verify this case is real and valid */
+ audit_filter_uring(tsk, context);
+ audit_filter_inodes(tsk, context);
+ if (context->current_state == AUDIT_STATE_RECORD)
+ audit_log_uring(context);
+ }
}
audit_set_context(tsk, NULL);
@@ -1875,12 +1910,6 @@ void __audit_uring_exit(int success, long code)
{
struct audit_context *ctx = audit_context();
- /*
- * TODO: At some point we will likely want to filter on io_uring ops
- * and other things similar to what we do for syscalls, but that
- * is something for another day; just record what we can here.
- */
-
if (ctx->context == AUDIT_CTX_SYSCALL) {
/*
* NOTE: See the note in __audit_uring_entry() about the case
@@ -1903,6 +1932,8 @@ void __audit_uring_exit(int success, long code)
* the behavior here.
*/
audit_filter_syscall(current, ctx);
+ if (ctx->current_state != AUDIT_STATE_RECORD)
+ audit_filter_uring(current, ctx);
audit_filter_inodes(current, ctx);
if (ctx->current_state != AUDIT_STATE_RECORD)
return;
@@ -1911,7 +1942,9 @@ void __audit_uring_exit(int success, long code)
return;
}
#if 1
- /* XXX - temporary hack to force record generation */
+ /* XXX - temporary hack to force record generation, we are leaving this
+ * enabled, but if you want to actually test the filtering you
+ * need to disable this #if/#endif block */
ctx->current_state = AUDIT_STATE_RECORD;
#endif
@@ -1919,6 +1952,8 @@ void __audit_uring_exit(int success, long code)
if (!list_empty(&ctx->killed_trees))
audit_kill_trees(ctx);
+ /* run through both filters to ensure we set the filterkey properly */
+ audit_filter_uring(current, ctx);
audit_filter_inodes(current, ctx);
if (ctx->current_state != AUDIT_STATE_RECORD)
goto out;
^ permalink raw reply related
* [RFC PATCH v2 3/9] audit: dev/test patch to force io_uring auditing
From: Paul Moore @ 2021-08-11 20:48 UTC (permalink / raw)
To: linux-security-module, selinux, linux-audit, io-uring,
linux-fsdevel, Kumar Kartikeya Dwivedi, Jens Axboe,
Pavel Begunkov
In-Reply-To: <162871480969.63873.9434591871437326374.stgit@olly>
WARNING - This patch is intended only to aid in the initial dev/test
of the audit/io_uring support, it is not intended to be merged.
With this patch, you can emit io_uring operation audit records with
the following commands (the first clears any blocking rules):
% auditctl -D
% auditctl -a exit,always -S io_uring_enter
Signed-off-by: DO NOT COMMIT
---
kernel/auditsc.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 62fb502da3fc..928f1dd12460 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1910,6 +1910,10 @@ void __audit_uring_exit(int success, long code)
audit_log_uring(ctx);
return;
}
+#if 1
+ /* XXX - temporary hack to force record generation */
+ ctx->current_state = AUDIT_STATE_RECORD;
+#endif
/* this may generate CONFIG_CHANGE records */
if (!list_empty(&ctx->killed_trees))
^ permalink raw reply related
* [RFC PATCH v2 2/9] audit,io_uring,io-wq: add some basic audit support to io_uring
From: Paul Moore @ 2021-08-11 20:48 UTC (permalink / raw)
To: linux-security-module, selinux, linux-audit, io-uring,
linux-fsdevel, Kumar Kartikeya Dwivedi, Jens Axboe,
Pavel Begunkov
In-Reply-To: <162871480969.63873.9434591871437326374.stgit@olly>
WARNING - This is a work in progress and should not be merged
anywhere important. It is almost surely not complete, and while it
probably compiles it likely hasn't been booted and will do terrible
things. You have been warned.
This patch adds basic auditing to io_uring operations, regardless of
their context. This is accomplished by allocating audit_context
structures for the io-wq worker and io_uring SQPOLL kernel threads
as well as explicitly auditing the io_uring operations in
io_issue_sqe(). The io_uring operations are audited using a new
AUDIT_URINGOP record, an example is shown below:
% <TODO - insert AUDIT_URINGOP record example>
Thanks to Richard Guy Briggs for review and feedback.
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
v2:
- added dummy funcs for audit_uring_{entry,exit}()
- replaced opcode checks in io_issue_sqe() with audit_skip checks
- moved fastpath checks into audit_uring_{entry,exit}()
- audit_log_uring() uses GFP_ATOMIC
- don't record the arch in __audit_uring_entry()
v1:
- initial draft
---
fs/io-wq.c | 4 +
fs/io_uring.c | 55 ++++++++++++--
include/linux/audit.h | 26 +++++++
include/uapi/linux/audit.h | 1
kernel/audit.h | 2 +
kernel/auditsc.c | 174 ++++++++++++++++++++++++++++++++++++++++++++
6 files changed, 256 insertions(+), 6 deletions(-)
diff --git a/fs/io-wq.c b/fs/io-wq.c
index 12fc19353bb0..fb928bac0bf2 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -14,6 +14,7 @@
#include <linux/rculist_nulls.h>
#include <linux/cpu.h>
#include <linux/tracehook.h>
+#include <linux/audit.h>
#include "io-wq.h"
@@ -552,6 +553,8 @@ static int io_wqe_worker(void *data)
snprintf(buf, sizeof(buf), "iou-wrk-%d", wq->task->pid);
set_task_comm(current, buf);
+ audit_alloc_kernel(current);
+
while (!test_bit(IO_WQ_BIT_EXIT, &wq->state)) {
long ret;
@@ -586,6 +589,7 @@ static int io_wqe_worker(void *data)
io_worker_handle_work(worker);
}
+ audit_free(current);
io_worker_exit(worker);
return 0;
}
diff --git a/fs/io_uring.c b/fs/io_uring.c
index bf548af0426c..b407a6ea1779 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -78,6 +78,7 @@
#include <linux/task_work.h>
#include <linux/pagemap.h>
#include <linux/io_uring.h>
+#include <linux/audit.h>
#define CREATE_TRACE_POINTS
#include <trace/events/io_uring.h>
@@ -896,6 +897,8 @@ struct io_op_def {
unsigned needs_async_setup : 1;
/* should block plug */
unsigned plug : 1;
+ /* skip auditing */
+ unsigned audit_skip : 1;
/* size of async data needed, if any */
unsigned short async_size;
};
@@ -909,6 +912,7 @@ static const struct io_op_def io_op_defs[] = {
.buffer_select = 1,
.needs_async_setup = 1,
.plug = 1,
+ .audit_skip = 1,
.async_size = sizeof(struct io_async_rw),
},
[IORING_OP_WRITEV] = {
@@ -918,16 +922,19 @@ static const struct io_op_def io_op_defs[] = {
.pollout = 1,
.needs_async_setup = 1,
.plug = 1,
+ .audit_skip = 1,
.async_size = sizeof(struct io_async_rw),
},
[IORING_OP_FSYNC] = {
.needs_file = 1,
+ .audit_skip = 1,
},
[IORING_OP_READ_FIXED] = {
.needs_file = 1,
.unbound_nonreg_file = 1,
.pollin = 1,
.plug = 1,
+ .audit_skip = 1,
.async_size = sizeof(struct io_async_rw),
},
[IORING_OP_WRITE_FIXED] = {
@@ -936,15 +943,20 @@ static const struct io_op_def io_op_defs[] = {
.unbound_nonreg_file = 1,
.pollout = 1,
.plug = 1,
+ .audit_skip = 1,
.async_size = sizeof(struct io_async_rw),
},
[IORING_OP_POLL_ADD] = {
.needs_file = 1,
.unbound_nonreg_file = 1,
+ .audit_skip = 1,
+ },
+ [IORING_OP_POLL_REMOVE] = {
+ .audit_skip = 1,
},
- [IORING_OP_POLL_REMOVE] = {},
[IORING_OP_SYNC_FILE_RANGE] = {
.needs_file = 1,
+ .audit_skip = 1,
},
[IORING_OP_SENDMSG] = {
.needs_file = 1,
@@ -962,18 +974,23 @@ static const struct io_op_def io_op_defs[] = {
.async_size = sizeof(struct io_async_msghdr),
},
[IORING_OP_TIMEOUT] = {
+ .audit_skip = 1,
.async_size = sizeof(struct io_timeout_data),
},
[IORING_OP_TIMEOUT_REMOVE] = {
/* used by timeout updates' prep() */
+ .audit_skip = 1,
},
[IORING_OP_ACCEPT] = {
.needs_file = 1,
.unbound_nonreg_file = 1,
.pollin = 1,
},
- [IORING_OP_ASYNC_CANCEL] = {},
+ [IORING_OP_ASYNC_CANCEL] = {
+ .audit_skip = 1,
+ },
[IORING_OP_LINK_TIMEOUT] = {
+ .audit_skip = 1,
.async_size = sizeof(struct io_timeout_data),
},
[IORING_OP_CONNECT] = {
@@ -988,14 +1005,19 @@ static const struct io_op_def io_op_defs[] = {
},
[IORING_OP_OPENAT] = {},
[IORING_OP_CLOSE] = {},
- [IORING_OP_FILES_UPDATE] = {},
- [IORING_OP_STATX] = {},
+ [IORING_OP_FILES_UPDATE] = {
+ .audit_skip = 1,
+ },
+ [IORING_OP_STATX] = {
+ .audit_skip = 1,
+ },
[IORING_OP_READ] = {
.needs_file = 1,
.unbound_nonreg_file = 1,
.pollin = 1,
.buffer_select = 1,
.plug = 1,
+ .audit_skip = 1,
.async_size = sizeof(struct io_async_rw),
},
[IORING_OP_WRITE] = {
@@ -1003,39 +1025,50 @@ static const struct io_op_def io_op_defs[] = {
.unbound_nonreg_file = 1,
.pollout = 1,
.plug = 1,
+ .audit_skip = 1,
.async_size = sizeof(struct io_async_rw),
},
[IORING_OP_FADVISE] = {
.needs_file = 1,
+ .audit_skip = 1,
},
[IORING_OP_MADVISE] = {},
[IORING_OP_SEND] = {
.needs_file = 1,
.unbound_nonreg_file = 1,
.pollout = 1,
+ .audit_skip = 1,
},
[IORING_OP_RECV] = {
.needs_file = 1,
.unbound_nonreg_file = 1,
.pollin = 1,
.buffer_select = 1,
+ .audit_skip = 1,
},
[IORING_OP_OPENAT2] = {
},
[IORING_OP_EPOLL_CTL] = {
.unbound_nonreg_file = 1,
+ .audit_skip = 1,
},
[IORING_OP_SPLICE] = {
.needs_file = 1,
.hash_reg_file = 1,
.unbound_nonreg_file = 1,
+ .audit_skip = 1,
+ },
+ [IORING_OP_PROVIDE_BUFFERS] = {
+ .audit_skip = 1,
+ },
+ [IORING_OP_REMOVE_BUFFERS] = {
+ .audit_skip = 1,
},
- [IORING_OP_PROVIDE_BUFFERS] = {},
- [IORING_OP_REMOVE_BUFFERS] = {},
[IORING_OP_TEE] = {
.needs_file = 1,
.hash_reg_file = 1,
.unbound_nonreg_file = 1,
+ .audit_skip = 1,
},
[IORING_OP_SHUTDOWN] = {
.needs_file = 1,
@@ -6166,6 +6199,9 @@ static int io_issue_sqe(struct io_kiocb *req, unsigned int issue_flags)
if ((req->flags & REQ_F_CREDS) && req->creds != current_cred())
creds = override_creds(req->creds);
+ if (!io_op_defs[req->opcode].audit_skip)
+ audit_uring_entry(req->opcode);
+
switch (req->opcode) {
case IORING_OP_NOP:
ret = io_nop(req, issue_flags);
@@ -6272,6 +6308,9 @@ static int io_issue_sqe(struct io_kiocb *req, unsigned int issue_flags)
break;
}
+ if (!io_op_defs[req->opcode].audit_skip)
+ audit_uring_exit(!ret, ret);
+
if (creds)
revert_creds(creds);
if (ret)
@@ -6896,6 +6935,8 @@ static int io_sq_thread(void *data)
set_cpus_allowed_ptr(current, cpu_online_mask);
current->flags |= PF_NO_SETAFFINITY;
+ audit_alloc_kernel(current);
+
mutex_lock(&sqd->lock);
while (1) {
bool cap_entries, sqt_spin = false;
@@ -6961,6 +7002,8 @@ static int io_sq_thread(void *data)
io_run_task_work();
mutex_unlock(&sqd->lock);
+ audit_free(current);
+
complete(&sqd->exited);
do_exit(0);
}
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 82b7c1116a85..d656a06dd909 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -286,7 +286,10 @@ static inline int audit_signal_info(int sig, struct task_struct *t)
/* These are defined in auditsc.c */
/* Public API */
extern int audit_alloc(struct task_struct *task);
+extern int audit_alloc_kernel(struct task_struct *task);
extern void __audit_free(struct task_struct *task);
+extern void __audit_uring_entry(u8 op);
+extern void __audit_uring_exit(int success, long code);
extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
unsigned long a2, unsigned long a3);
extern void __audit_syscall_exit(int ret_success, long ret_value);
@@ -323,6 +326,21 @@ static inline void audit_free(struct task_struct *task)
if (unlikely(task->audit_context))
__audit_free(task);
}
+static inline void audit_uring_entry(u8 op)
+{
+ /*
+ * We intentionally check audit_context() before audit_enabled as most
+ * Linux systems (as of ~2021) rely on systemd which forces audit to
+ * be enabled regardless of the user's audit configuration.
+ */
+ if (unlikely(audit_context() && audit_enabled))
+ __audit_uring_entry(op);
+}
+static inline void audit_uring_exit(int success, long code)
+{
+ if (unlikely(!audit_dummy_context()))
+ __audit_uring_exit(success, code);
+}
static inline void audit_syscall_entry(int major, unsigned long a0,
unsigned long a1, unsigned long a2,
unsigned long a3)
@@ -554,8 +572,16 @@ static inline int audit_alloc(struct task_struct *task)
{
return 0;
}
+static inline int audit_alloc_kernel(struct task_struct *task)
+{
+ return 0;
+}
static inline void audit_free(struct task_struct *task)
{ }
+static inline void audit_uring_entry(u8 op)
+{ }
+static inline void audit_uring_exit(int success, long code)
+{ }
static inline void audit_syscall_entry(int major, unsigned long a0,
unsigned long a1, unsigned long a2,
unsigned long a3)
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index daa481729e9b..a1997697c8b1 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -118,6 +118,7 @@
#define AUDIT_TIME_ADJNTPVAL 1333 /* NTP value adjustment */
#define AUDIT_BPF 1334 /* BPF subsystem */
#define AUDIT_EVENT_LISTENER 1335 /* Task joined multicast read socket */
+#define AUDIT_URINGOP 1336 /* io_uring operation */
#define AUDIT_AVC 1400 /* SE Linux avc denial or grant */
#define AUDIT_SELINUX_ERR 1401 /* Internal SE Linux Errors */
diff --git a/kernel/audit.h b/kernel/audit.h
index 268a13a1c983..834fa18fd6ca 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -100,10 +100,12 @@ struct audit_context {
enum {
AUDIT_CTX_UNUSED, /* audit_context is currently unused */
AUDIT_CTX_SYSCALL, /* in use by syscall */
+ AUDIT_CTX_URING, /* in use by io_uring */
} context;
enum audit_state state, current_state;
unsigned int serial; /* serial number for record */
int major; /* syscall number */
+ int uring_op; /* uring operation */
struct timespec64 ctime; /* time of syscall entry */
unsigned long argv[4]; /* syscall arguments */
long return_code;/* syscall return code */
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index c0383d554e61..62fb502da3fc 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -959,6 +959,7 @@ static void audit_reset_context(struct audit_context *ctx)
ctx->current_state = ctx->state;
ctx->serial = 0;
ctx->major = 0;
+ ctx->uring_op = 0;
ctx->ctime = (struct timespec64){ .tv_sec = 0, .tv_nsec = 0 };
memset(ctx->argv, 0, sizeof(ctx->argv));
ctx->return_code = 0;
@@ -1044,6 +1045,31 @@ int audit_alloc(struct task_struct *tsk)
return 0;
}
+/**
+ * audit_alloc_kernel - allocate an audit_context for a kernel task
+ * @tsk: the kernel task
+ *
+ * Similar to the audit_alloc() function, but intended for kernel private
+ * threads. Returns zero on success, negative values on failure.
+ */
+int audit_alloc_kernel(struct task_struct *tsk)
+{
+ /*
+ * At the moment we are just going to call into audit_alloc() to
+ * simplify the code, but there two things to keep in mind with this
+ * approach:
+ *
+ * 1. Filtering internal kernel tasks is a bit laughable in almost all
+ * cases, but there is at least one case where there is a benefit:
+ * the '-a task,never' case allows the admin to effectively disable
+ * task auditing at runtime.
+ *
+ * 2. The {set,clear}_task_syscall_work() ops likely have zero effect
+ * on these internal kernel tasks, but they probably don't hurt either.
+ */
+ return audit_alloc(tsk);
+}
+
static inline void audit_free_context(struct audit_context *context)
{
/* resetting is extra work, but it is likely just noise */
@@ -1546,6 +1572,52 @@ static void audit_log_proctitle(void)
audit_log_end(ab);
}
+/**
+ * audit_log_uring - generate a AUDIT_URINGOP record
+ * @ctx: the audit context
+ */
+static void audit_log_uring(struct audit_context *ctx)
+{
+ struct audit_buffer *ab;
+ const struct cred *cred;
+
+ /*
+ * TODO: What do we log here? I'm tossing in a few things to start the
+ * conversation, but additional thought needs to go into this.
+ */
+
+ ab = audit_log_start(ctx, GFP_ATOMIC, AUDIT_URINGOP);
+ if (!ab)
+ return;
+ cred = current_cred();
+ audit_log_format(ab, "uring_op=%d", ctx->uring_op);
+ if (ctx->return_valid != AUDITSC_INVALID)
+ audit_log_format(ab, " success=%s exit=%ld",
+ (ctx->return_valid == AUDITSC_SUCCESS ?
+ "yes" : "no"),
+ ctx->return_code);
+ audit_log_format(ab,
+ " items=%d"
+ " ppid=%d pid=%d auid=%u uid=%u gid=%u"
+ " euid=%u suid=%u fsuid=%u"
+ " egid=%u sgid=%u fsgid=%u",
+ ctx->name_count,
+ task_ppid_nr(current),
+ task_tgid_nr(current),
+ from_kuid(&init_user_ns, audit_get_loginuid(current)),
+ from_kuid(&init_user_ns, cred->uid),
+ from_kgid(&init_user_ns, cred->gid),
+ from_kuid(&init_user_ns, cred->euid),
+ from_kuid(&init_user_ns, cred->suid),
+ from_kuid(&init_user_ns, cred->fsuid),
+ from_kgid(&init_user_ns, cred->egid),
+ from_kgid(&init_user_ns, cred->sgid),
+ from_kgid(&init_user_ns, cred->fsgid));
+ audit_log_task_context(ab);
+ audit_log_key(ab, ctx->filterkey);
+ audit_log_end(ab);
+}
+
static void audit_log_exit(void)
{
int i, call_panic = 0;
@@ -1581,6 +1653,9 @@ static void audit_log_exit(void)
audit_log_key(ab, context->filterkey);
audit_log_end(ab);
break;
+ case AUDIT_CTX_URING:
+ audit_log_uring(context);
+ break;
default:
BUG();
break;
@@ -1751,6 +1826,105 @@ static void audit_return_fixup(struct audit_context *ctx,
ctx->return_valid = (success ? AUDITSC_SUCCESS : AUDITSC_FAILURE);
}
+/**
+ * __audit_uring_entry - prepare the kernel task's audit context for io_uring
+ * @op: the io_uring opcode
+ *
+ * This is similar to audit_syscall_entry() but is intended for use by io_uring
+ * operations. This function should only ever be called from
+ * audit_uring_entry() as we rely on the audit context checking present in that
+ * function.
+ */
+void __audit_uring_entry(u8 op)
+{
+ struct audit_context *ctx = audit_context();
+
+ if (ctx->state == AUDIT_STATE_DISABLED)
+ return;
+
+ /*
+ * NOTE: It's possible that we can be called from the process' context
+ * before it returns to userspace, and before audit_syscall_exit()
+ * is called. In this case there is not much to do, just record
+ * the io_uring details and return.
+ */
+ ctx->uring_op = op;
+ if (ctx->context == AUDIT_CTX_SYSCALL)
+ return;
+
+ ctx->dummy = !audit_n_rules;
+ if (!ctx->dummy && ctx->state == AUDIT_STATE_BUILD)
+ ctx->prio = 0;
+
+ ctx->context = AUDIT_CTX_URING;
+ ctx->current_state = ctx->state;
+ ktime_get_coarse_real_ts64(&ctx->ctime);
+}
+
+/**
+ * __audit_uring_exit - wrap up the kernel task's audit context after io_uring
+ * @success: true/false value to indicate if the operation succeeded or not
+ * @code: operation return code
+ *
+ * This is similar to audit_syscall_exit() but is intended for use by io_uring
+ * operations. This function should only ever be called from
+ * audit_uring_exit() as we rely on the audit context checking present in that
+ * function.
+ */
+void __audit_uring_exit(int success, long code)
+{
+ struct audit_context *ctx = audit_context();
+
+ /*
+ * TODO: At some point we will likely want to filter on io_uring ops
+ * and other things similar to what we do for syscalls, but that
+ * is something for another day; just record what we can here.
+ */
+
+ if (ctx->context == AUDIT_CTX_SYSCALL) {
+ /*
+ * NOTE: See the note in __audit_uring_entry() about the case
+ * where we may be called from process context before we
+ * return to userspace via audit_syscall_exit(). In this
+ * case we simply emit a URINGOP record and bail, the
+ * normal syscall exit handling will take care of
+ * everything else.
+ * It is also worth mentioning that when we are called,
+ * the current process creds may differ from the creds
+ * used during the normal syscall processing; keep that
+ * in mind if/when we move the record generation code.
+ */
+
+ /*
+ * We need to filter on the syscall info here to decide if we
+ * should emit a URINGOP record. I know it seems odd but this
+ * solves the problem where users have a filter to block *all*
+ * syscall records in the "exit" filter; we want to preserve
+ * the behavior here.
+ */
+ audit_filter_syscall(current, ctx);
+ audit_filter_inodes(current, ctx);
+ if (ctx->current_state != AUDIT_STATE_RECORD)
+ return;
+
+ audit_log_uring(ctx);
+ return;
+ }
+
+ /* this may generate CONFIG_CHANGE records */
+ if (!list_empty(&ctx->killed_trees))
+ audit_kill_trees(ctx);
+
+ audit_filter_inodes(current, ctx);
+ if (ctx->current_state != AUDIT_STATE_RECORD)
+ goto out;
+ audit_return_fixup(ctx, success, code);
+ audit_log_exit();
+
+out:
+ audit_reset_context(ctx);
+}
+
/**
* __audit_syscall_entry - fill in an audit record at syscall entry
* @major: major syscall type (function)
^ permalink raw reply related
* [RFC PATCH v2 1/9] audit: prepare audit_context for use in calling contexts beyond syscalls
From: Paul Moore @ 2021-08-11 20:48 UTC (permalink / raw)
To: linux-security-module, selinux, linux-audit, io-uring,
linux-fsdevel, Kumar Kartikeya Dwivedi, Jens Axboe,
Pavel Begunkov
In-Reply-To: <162871480969.63873.9434591871437326374.stgit@olly>
WARNING - This is a work in progress and should not be merged
anywhere important. It is almost surely not complete, and while it
probably compiles it likely hasn't been booted and will do terrible
things. You have been warned.
This patch cleans up some of our audit_context handling by
abstracting out the reset and return code fixup handling to dedicated
functions. Not only does this help make things easier to read and
inspect, it allows for easier reuse be future patches. We also
convert the simple audit_context->in_syscall flag into an enum which
can be used to by future patches to indicate a calling context other
than the syscall context.
Thanks to Richard Guy Briggs for review and feedback.
Acked-by: Richard Guy Briggs <rgb@redhat.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
v2:
- no change
v1:
- initial draft
---
kernel/audit.h | 5 +
kernel/auditsc.c | 256 ++++++++++++++++++++++++++++++++++--------------------
2 files changed, 167 insertions(+), 94 deletions(-)
diff --git a/kernel/audit.h b/kernel/audit.h
index b565ea16c0a5..268a13a1c983 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -97,7 +97,10 @@ struct audit_proctitle {
/* The per-task audit context. */
struct audit_context {
int dummy; /* must be the first element */
- int in_syscall; /* 1 if task is in a syscall */
+ enum {
+ AUDIT_CTX_UNUSED, /* audit_context is currently unused */
+ AUDIT_CTX_SYSCALL, /* in use by syscall */
+ } context;
enum audit_state state, current_state;
unsigned int serial; /* serial number for record */
int major; /* syscall number */
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 8dd73a64f921..c0383d554e61 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -915,10 +915,80 @@ static inline void audit_free_aux(struct audit_context *context)
context->aux = aux->next;
kfree(aux);
}
+ context->aux = NULL;
while ((aux = context->aux_pids)) {
context->aux_pids = aux->next;
kfree(aux);
}
+ context->aux_pids = NULL;
+}
+
+/**
+ * audit_reset_context - reset a audit_context structure
+ * @ctx: the audit_context to reset
+ *
+ * All fields in the audit_context will be reset to an initial state, all
+ * references held by fields will be dropped, and private memory will be
+ * released. When this function returns the audit_context will be suitable
+ * for reuse, so long as the passed context is not NULL or a dummy context.
+ */
+static void audit_reset_context(struct audit_context *ctx)
+{
+ if (!ctx)
+ return;
+
+ /* if ctx is non-null, reset the "ctx->state" regardless */
+ ctx->context = AUDIT_CTX_UNUSED;
+ if (ctx->dummy)
+ return;
+
+ /*
+ * NOTE: It shouldn't matter in what order we release the fields, so
+ * release them in the order in which they appear in the struct;
+ * this gives us some hope of quickly making sure we are
+ * resetting the audit_context properly.
+ *
+ * Other things worth mentioning:
+ * - we don't reset "dummy"
+ * - we don't reset "state", we do reset "current_state"
+ * - we preserver "filterkey" if "state" is AUDIT_STATE_RECORD
+ * - much of this is likely overkill, but play it safe for now
+ * - we really need to work on improving the audit_context struct
+ */
+
+ ctx->current_state = ctx->state;
+ ctx->serial = 0;
+ ctx->major = 0;
+ ctx->ctime = (struct timespec64){ .tv_sec = 0, .tv_nsec = 0 };
+ memset(ctx->argv, 0, sizeof(ctx->argv));
+ ctx->return_code = 0;
+ ctx->prio = (ctx->state == AUDIT_STATE_RECORD ? ~0ULL : 0);
+ ctx->return_valid = AUDITSC_INVALID;
+ audit_free_names(ctx);
+ if (ctx->state != AUDIT_STATE_RECORD) {
+ kfree(ctx->filterkey);
+ ctx->filterkey = NULL;
+ }
+ audit_free_aux(ctx);
+ kfree(ctx->sockaddr);
+ ctx->sockaddr = NULL;
+ ctx->sockaddr_len = 0;
+ ctx->pid = ctx->ppid = 0;
+ ctx->uid = ctx->euid = ctx->suid = ctx->fsuid = KUIDT_INIT(0);
+ ctx->gid = ctx->egid = ctx->sgid = ctx->fsgid = KGIDT_INIT(0);
+ ctx->personality = 0;
+ ctx->arch = 0;
+ ctx->target_pid = 0;
+ ctx->target_auid = ctx->target_uid = KUIDT_INIT(0);
+ ctx->target_sessionid = 0;
+ ctx->target_sid = 0;
+ ctx->target_comm[0] = '\0';
+ unroll_tree_refs(ctx, NULL, 0);
+ WARN_ON(!list_empty(&ctx->killed_trees));
+ ctx->type = 0;
+ audit_free_module(ctx);
+ ctx->fds[0] = -1;
+ audit_proctitle_free(ctx);
}
static inline struct audit_context *audit_alloc_context(enum audit_state state)
@@ -928,6 +998,7 @@ static inline struct audit_context *audit_alloc_context(enum audit_state state)
context = kzalloc(sizeof(*context), GFP_KERNEL);
if (!context)
return NULL;
+ context->context = AUDIT_CTX_UNUSED;
context->state = state;
context->prio = state == AUDIT_STATE_RECORD ? ~0ULL : 0;
INIT_LIST_HEAD(&context->killed_trees);
@@ -953,7 +1024,7 @@ int audit_alloc(struct task_struct *tsk)
char *key = NULL;
if (likely(!audit_ever_enabled))
- return 0; /* Return if not auditing. */
+ return 0;
state = audit_filter_task(tsk, &key);
if (state == AUDIT_STATE_DISABLED) {
@@ -975,14 +1046,10 @@ int audit_alloc(struct task_struct *tsk)
static inline void audit_free_context(struct audit_context *context)
{
- audit_free_module(context);
- audit_free_names(context);
- unroll_tree_refs(context, NULL, 0);
+ /* resetting is extra work, but it is likely just noise */
+ audit_reset_context(context);
free_tree_refs(context);
- audit_free_aux(context);
kfree(context->filterkey);
- kfree(context->sockaddr);
- audit_proctitle_free(context);
kfree(context);
}
@@ -1489,29 +1556,35 @@ static void audit_log_exit(void)
context->personality = current->personality;
- ab = audit_log_start(context, GFP_KERNEL, AUDIT_SYSCALL);
- if (!ab)
- return; /* audit_panic has been called */
- audit_log_format(ab, "arch=%x syscall=%d",
- context->arch, context->major);
- if (context->personality != PER_LINUX)
- audit_log_format(ab, " per=%lx", context->personality);
- if (context->return_valid != AUDITSC_INVALID)
- audit_log_format(ab, " success=%s exit=%ld",
- (context->return_valid==AUDITSC_SUCCESS)?"yes":"no",
- context->return_code);
-
- audit_log_format(ab,
- " a0=%lx a1=%lx a2=%lx a3=%lx items=%d",
- context->argv[0],
- context->argv[1],
- context->argv[2],
- context->argv[3],
- context->name_count);
-
- audit_log_task_info(ab);
- audit_log_key(ab, context->filterkey);
- audit_log_end(ab);
+ switch (context->context) {
+ case AUDIT_CTX_SYSCALL:
+ ab = audit_log_start(context, GFP_KERNEL, AUDIT_SYSCALL);
+ if (!ab)
+ return;
+ audit_log_format(ab, "arch=%x syscall=%d",
+ context->arch, context->major);
+ if (context->personality != PER_LINUX)
+ audit_log_format(ab, " per=%lx", context->personality);
+ if (context->return_valid != AUDITSC_INVALID)
+ audit_log_format(ab, " success=%s exit=%ld",
+ (context->return_valid == AUDITSC_SUCCESS ?
+ "yes" : "no"),
+ context->return_code);
+ audit_log_format(ab,
+ " a0=%lx a1=%lx a2=%lx a3=%lx items=%d",
+ context->argv[0],
+ context->argv[1],
+ context->argv[2],
+ context->argv[3],
+ context->name_count);
+ audit_log_task_info(ab);
+ audit_log_key(ab, context->filterkey);
+ audit_log_end(ab);
+ break;
+ default:
+ BUG();
+ break;
+ }
for (aux = context->aux; aux; aux = aux->next) {
@@ -1602,14 +1675,15 @@ static void audit_log_exit(void)
audit_log_name(context, n, NULL, i++, &call_panic);
}
- audit_log_proctitle();
+ if (context->context == AUDIT_CTX_SYSCALL)
+ audit_log_proctitle();
/* Send end of event record to help user space know we are finished */
ab = audit_log_start(context, GFP_KERNEL, AUDIT_EOE);
if (ab)
audit_log_end(ab);
if (call_panic)
- audit_panic("error converting sid to string");
+ audit_panic("error in audit_log_exit()");
}
/**
@@ -1625,6 +1699,7 @@ void __audit_free(struct task_struct *tsk)
if (!context)
return;
+ /* this may generate CONFIG_CHANGE records */
if (!list_empty(&context->killed_trees))
audit_kill_trees(context);
@@ -1633,7 +1708,8 @@ void __audit_free(struct task_struct *tsk)
* random task_struct that doesn't doesn't have any meaningful data we
* need to log via audit_log_exit().
*/
- if (tsk == current && !context->dummy && context->in_syscall) {
+ if (tsk == current && !context->dummy &&
+ context->context == AUDIT_CTX_SYSCALL) {
context->return_valid = AUDITSC_INVALID;
context->return_code = 0;
@@ -1647,6 +1723,34 @@ void __audit_free(struct task_struct *tsk)
audit_free_context(context);
}
+/**
+ * audit_return_fixup - fixup the return codes in the audit_context
+ * @ctx: the audit_context
+ * @success: true/false value to indicate if the operation succeeded or not
+ * @code: operation return code
+ *
+ * We need to fixup the return code in the audit logs if the actual return
+ * codes are later going to be fixed by the arch specific signal handlers.
+ */
+static void audit_return_fixup(struct audit_context *ctx,
+ int success, long code)
+{
+ /*
+ * This is actually a test for:
+ * (rc == ERESTARTSYS ) || (rc == ERESTARTNOINTR) ||
+ * (rc == ERESTARTNOHAND) || (rc == ERESTART_RESTARTBLOCK)
+ *
+ * but is faster than a bunch of ||
+ */
+ if (unlikely(code <= -ERESTARTSYS) &&
+ (code >= -ERESTART_RESTARTBLOCK) &&
+ (code != -ENOIOCTLCMD))
+ ctx->return_code = -EINTR;
+ else
+ ctx->return_code = code;
+ ctx->return_valid = (success ? AUDITSC_SUCCESS : AUDITSC_FAILURE);
+}
+
/**
* __audit_syscall_entry - fill in an audit record at syscall entry
* @major: major syscall type (function)
@@ -1672,7 +1776,12 @@ void __audit_syscall_entry(int major, unsigned long a1, unsigned long a2,
if (!audit_enabled || !context)
return;
- BUG_ON(context->in_syscall || context->name_count);
+ WARN_ON(context->context != AUDIT_CTX_UNUSED);
+ WARN_ON(context->name_count);
+ if (context->context != AUDIT_CTX_UNUSED || context->name_count) {
+ audit_panic("unrecoverable error in audit_syscall_entry()");
+ return;
+ }
state = context->state;
if (state == AUDIT_STATE_DISABLED)
@@ -1691,10 +1800,8 @@ void __audit_syscall_entry(int major, unsigned long a1, unsigned long a2,
context->argv[1] = a2;
context->argv[2] = a3;
context->argv[3] = a4;
- context->serial = 0;
- context->in_syscall = 1;
+ context->context = AUDIT_CTX_SYSCALL;
context->current_state = state;
- context->ppid = 0;
ktime_get_coarse_real_ts64(&context->ctime);
}
@@ -1711,63 +1818,27 @@ void __audit_syscall_entry(int major, unsigned long a1, unsigned long a2,
*/
void __audit_syscall_exit(int success, long return_code)
{
- struct audit_context *context;
+ struct audit_context *context = audit_context();
- context = audit_context();
- if (!context)
- return;
+ if (!context || context->dummy ||
+ context->context != AUDIT_CTX_SYSCALL)
+ goto out;
+ /* this may generate CONFIG_CHANGE records */
if (!list_empty(&context->killed_trees))
audit_kill_trees(context);
- if (!context->dummy && context->in_syscall) {
- if (success)
- context->return_valid = AUDITSC_SUCCESS;
- else
- context->return_valid = AUDITSC_FAILURE;
-
- /*
- * we need to fix up the return code in the audit logs if the
- * actual return codes are later going to be fixed up by the
- * arch specific signal handlers
- *
- * This is actually a test for:
- * (rc == ERESTARTSYS ) || (rc == ERESTARTNOINTR) ||
- * (rc == ERESTARTNOHAND) || (rc == ERESTART_RESTARTBLOCK)
- *
- * but is faster than a bunch of ||
- */
- if (unlikely(return_code <= -ERESTARTSYS) &&
- (return_code >= -ERESTART_RESTARTBLOCK) &&
- (return_code != -ENOIOCTLCMD))
- context->return_code = -EINTR;
- else
- context->return_code = return_code;
-
- audit_filter_syscall(current, context);
- audit_filter_inodes(current, context);
- if (context->current_state == AUDIT_STATE_RECORD)
- audit_log_exit();
- }
+ /* run through both filters to ensure we set the filterkey properly */
+ audit_filter_syscall(current, context);
+ audit_filter_inodes(current, context);
+ if (context->current_state < AUDIT_STATE_RECORD)
+ goto out;
- context->in_syscall = 0;
- context->prio = context->state == AUDIT_STATE_RECORD ? ~0ULL : 0;
+ audit_return_fixup(context, success, return_code);
+ audit_log_exit();
- audit_free_module(context);
- audit_free_names(context);
- unroll_tree_refs(context, NULL, 0);
- audit_free_aux(context);
- context->aux = NULL;
- context->aux_pids = NULL;
- context->target_pid = 0;
- context->target_sid = 0;
- context->sockaddr_len = 0;
- context->type = 0;
- context->fds[0] = -1;
- if (context->state != AUDIT_STATE_RECORD) {
- kfree(context->filterkey);
- context->filterkey = NULL;
- }
+out:
+ audit_reset_context(context);
}
static inline void handle_one(const struct inode *inode)
@@ -1919,7 +1990,7 @@ void __audit_getname(struct filename *name)
struct audit_context *context = audit_context();
struct audit_names *n;
- if (!context->in_syscall)
+ if (context->context == AUDIT_CTX_UNUSED)
return;
n = audit_alloc_name(context, AUDIT_TYPE_UNKNOWN);
@@ -1991,7 +2062,7 @@ void __audit_inode(struct filename *name, const struct dentry *dentry,
struct list_head *list = &audit_filter_list[AUDIT_FILTER_FS];
int i;
- if (!context->in_syscall)
+ if (context->context == AUDIT_CTX_UNUSED)
return;
rcu_read_lock();
@@ -2109,7 +2180,7 @@ void __audit_inode_child(struct inode *parent,
struct list_head *list = &audit_filter_list[AUDIT_FILTER_FS];
int i;
- if (!context->in_syscall)
+ if (context->context == AUDIT_CTX_UNUSED)
return;
rcu_read_lock();
@@ -2208,7 +2279,7 @@ EXPORT_SYMBOL_GPL(__audit_inode_child);
int auditsc_get_stamp(struct audit_context *ctx,
struct timespec64 *t, unsigned int *serial)
{
- if (!ctx->in_syscall)
+ if (ctx->context == AUDIT_CTX_UNUSED)
return 0;
if (!ctx->serial)
ctx->serial = audit_serial();
@@ -2706,8 +2777,7 @@ void audit_seccomp_actions_logged(const char *names, const char *old_names,
struct list_head *audit_killed_trees(void)
{
struct audit_context *ctx = audit_context();
-
- if (likely(!ctx || !ctx->in_syscall))
+ if (likely(!ctx || ctx->context == AUDIT_CTX_UNUSED))
return NULL;
return &ctx->killed_trees;
}
^ permalink raw reply related
* [RFC PATCH v2 0/9] Add LSM access controls and auditing to io_uring
From: Paul Moore @ 2021-08-11 20:48 UTC (permalink / raw)
To: linux-security-module, selinux, linux-audit, io-uring,
linux-fsdevel, Kumar Kartikeya Dwivedi, Jens Axboe,
Pavel Begunkov
Draft #2 of the patchset which brings auditing and proper LSM access
controls to the io_uring subsystem. The original patchset was posted
in late May and can be found via lore using the link below:
https://lore.kernel.org/linux-security-module/162163367115.8379.8459012634106035341.stgit@sifl/
This draft should incorporate all of the feedback from the original
posting as well as a few smaller things I noticed while playing
further with the code. The big change is of course the selective
auditing in the io_uring op servicing, but that has already been
discussed quite a bit in the original thread so I won't go into
detail here; the important part is that we found a way to move
forward and this draft captures that. For those of you looking to
play with these patches, they are based on Linus' v5.14-rc5 tag and
on my test system they boot and appear to function without problem;
they pass the selinux-testsuite and audit-testsuite and I have not
noticed any regressions in the normal use of the system. If you want
to get a copy of these patches straight from git you can use the
"working-io_uring" branch in the repo below:
git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
Beyond the existing test suite tests mentioned above, I've cobbled
together some very basic, very crude tests to exercise some of the
things I care about from a LSM/audit perspective. These tests are
pretty awful (I'm not kidding), but they might be helpful for the
other LSM/audit developers who want to test things:
https://drop.paul-moore.com/90.kUgq
There are currently two tests: 'iouring.2' and 'iouring.3';
'iouring.1' was lost in a misguided and overzealous 'rm' command.
The first test is standalone and basically tests the SQPOLL
functionality while the second tests sharing io_urings across process
boundaries and the credential/personality sharing mechanism. The
console output of both tests isn't particularly useful, the more
interesting bits are in the audit and LSM specific logs. The
'iouring.2' command requires no special arguments to run but the
'iouring.3' test is split into a "server" and "client"; the server
should be run without argument:
% ./iouring.3s
>>> server started, pid = 11678
>>> memfd created, fd = 3
>>> io_uring created; fd = 5, creds = 1
... while the client should be run with two arguments: the first is
the PID of the server process, the second is the "memfd" fd number:
% ./iouring.3c 11678 3
>>> client started, server_pid = 11678 server_memfd = 3
>>> io_urings = 5 (server) / 5 (client)
>>> io_uring ops using creds = 1
>>> async op result: 36
>>> async op result: 36
>>> async op result: 36
>>> async op result: 36
>>> START file contents
What is this life if, full of care,
we have no time to stand and stare.
>>> END file contents
The tests were hacked together from various sources online,
attribution and links to additional info can be found in the test
sources, but I expect these tests to die a fiery death in the not
to distant future as I work to add some proper tests to the SELinux
and audit test suites.
As I believe these patches should spend a full -rcX cycle in
linux-next, my current plan is to continue to solicit feedback on
these patches while they undergo additional testing (next up is
verification of the audit filter code for io_uring). Assuming no
critical issues are found on the mailing lists or during testing, I
will post a proper patchset later with the idea of merging it into
selinux/next after the upcoming merge window closes.
Any comments, feedback, etc. are welcome.
---
Casey Schaufler (1):
Smack: Brutalist io_uring support with debug
Paul Moore (8):
audit: prepare audit_context for use in calling contexts beyond
syscalls
audit,io_uring,io-wq: add some basic audit support to io_uring
audit: dev/test patch to force io_uring auditing
audit: add filtering for io_uring records
fs: add anon_inode_getfile_secure() similar to
anon_inode_getfd_secure()
io_uring: convert io_uring to the secure anon inode interface
lsm,io_uring: add LSM hooks to io_uring
selinux: add support for the io_uring access controls
fs/anon_inodes.c | 29 ++
fs/io-wq.c | 4 +
fs/io_uring.c | 69 +++-
include/linux/anon_inodes.h | 4 +
include/linux/audit.h | 26 ++
include/linux/lsm_hook_defs.h | 5 +
include/linux/lsm_hooks.h | 13 +
include/linux/security.h | 16 +
include/uapi/linux/audit.h | 4 +-
kernel/audit.h | 7 +-
kernel/audit_tree.c | 3 +-
kernel/audit_watch.c | 3 +-
kernel/auditfilter.c | 15 +-
kernel/auditsc.c | 483 +++++++++++++++++++-----
security/security.c | 12 +
security/selinux/hooks.c | 34 ++
security/selinux/include/classmap.h | 2 +
security/smack/smack_lsm.c | 64 ++++
18 files changed, 678 insertions(+), 115 deletions(-)
^ permalink raw reply
* Re: [PATCH v2] fscrypt: support trusted keys
From: Eric Biggers @ 2021-08-11 17:16 UTC (permalink / raw)
To: Mimi Zohar
Cc: Jarkko Sakkinen, Ahmad Fatoum, Theodore Y. Ts'o, Jaegeuk Kim,
kernel, James Morris, Serge E. Hallyn, James Bottomley,
Sumit Garg, David Howells, linux-fscrypt, linux-crypto,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <d4f5c2593380c82ceebae2c8782a1c440b35f165.camel@linux.ibm.com>
On Wed, Aug 11, 2021 at 07:34:18AM -0400, Mimi Zohar wrote:
> On Wed, 2021-08-11 at 03:17 +0300, Jarkko Sakkinen wrote:
> > On Tue, Aug 10, 2021 at 02:27:24PM -0700, Eric Biggers wrote:
> > > On Wed, Aug 11, 2021 at 12:21:40AM +0300, Jarkko Sakkinen wrote:
> > > > On Tue, Aug 10, 2021 at 11:46:49AM -0700, Eric Biggers wrote:
> > > > > On Tue, Aug 10, 2021 at 09:06:36PM +0300, Jarkko Sakkinen wrote:
> > > > > > > >
> > > > > > > > I don't think this is right, or at least it does not follow the pattern
> > > > > > > > in [*]. I.e. you should rather use trusted key to seal your fscrypt key.
> > > > > > >
> > > > > > > What's the benefit of the extra layer of indirection over just using a "trusted"
> > > > > > > key directly? The use case for "encrypted" keys is not at all clear to me.
> > > > > >
> > > > > > Because it is more robust to be able to use small amount of trusted keys,
> > > > > > which are not entirely software based.
> > > > > >
> > > > > > And since it's also a pattern on existing kernel features utilizing trusted
> > > > > > keys, the pressure here to explain why break the pattern, should be on the
> > > > > > side of the one who breaks it.
> > > > >
> > > > > This is a new feature, so it's on the person proposing the feature to explain
> > > > > why it's useful. The purpose of "encrypted" keys is not at all clear, and the
> > > > > documentation for them is heavily misleading. E.g.:
> > > > >
> > > > > "user space sees, stores, and loads only encrypted blobs"
> > > > > (Not necessarily true, as I've explained previously.)
> > > > >
> > > > > "Encrypted keys do not depend on a trust source" ... "The main disadvantage
> > > > > of encrypted keys is that if they are not rooted in a trusted key"
> > > > > (Not necessarily true, and in fact it seems they're only useful when they
> > > > > *do* depend on a trust source. At least that's the use case that is being
> > > > > proposed here, IIUC.)
> > > > >
> > > > > I do see a possible use for the layer of indirection that "encrypted" keys are,
> > > > > which is that it would reduce the overhead of having lots of keys be directly
> > > > > encrypted by the TPM/TEE/CAAM. Is this the use case? If so, it needs to be
> > > > > explained.
> > > >
> > > > If trusted keys are used directly, it's an introduction of a bottleneck.
> > > > If they are used indirectly, you can still choose to have one trusted
> > > > key per fscrypt key.
> > > >
> > > > Thus, it's obviously a bad idea to use them directly.
> > >
> > > So actually explain that in the documentation. It's not obvious at all. And
> > > does this imply that the support for trusted keys in dm-crypt is a mistake?
> >
> > Looking at dm-crypt implementation, you can choose to use 'encrypted' key
> > type, which you can seal with a trusted key.
> >
> > Note: I have not been involved when the feature was added to dm-crypt.
>
> At least for TPM 1.2, "trusted" keys may be sealed to a PCR and then
> extended to prevent subsequent usage. For example, in the initramfs
> all of the "encrypted" keys could be decrypted by a single "trusted"
> key, before extending the PCR.
>
> Mimi
>
Neither of you actually answered my question, which is whether the support for
trusted keys in dm-crypt is a mistake. I think you're saying that it is? That
would imply that fscrypt shouldn't support trusted keys, but rather encrypted
keys -- which conflicts with Ahmad's patch which is adding support for trusted
keys. Note that your reasoning for this is not documented at all in the
trusted-encrypted keys documentation; it needs to be (email threads don't really
matter), otherwise how would anyone know when/how to use this feature?
- Eric
^ permalink raw reply
* Re: [PATCH v4 2/2] tpm: ibmvtpm: Rename tpm_process_cmd to tpm_status and define flag
From: Stefan Berger @ 2021-08-11 12:15 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Stefan Berger, nasastry, linux-integrity, linux-security-module,
linux-kernel, Nayna Jain, George Wilson
In-Reply-To: <20210811021030.5meaty2zxf253nfl@kernel.org>
On 8/10/21 10:10 PM, Jarkko Sakkinen wrote:
> On Tue, Aug 10, 2021 at 09:50:55PM -0400, Stefan Berger wrote:
>> On 8/10/21 1:58 PM, Jarkko Sakkinen wrote:
>>> On Mon, Aug 09, 2021 at 03:21:59PM -0400, Stefan Berger wrote:
>>>> From: Stefan Berger <stefanb@linux.ibm.com>
>>>>
>>>> Rename the field tpm_processing_cmd to tpm_status in ibmvtpm_dev and set
>>>> the TPM_STATUS_BUSY flag while the vTPM is busy processing a command.
>>>>
>>>>
>>>> default:
>>>> diff --git a/drivers/char/tpm/tpm_ibmvtpm.h b/drivers/char/tpm/tpm_ibmvtpm.h
>>>> index 51198b137461..252f1cccdfc5 100644
>>>> --- a/drivers/char/tpm/tpm_ibmvtpm.h
>>>> +++ b/drivers/char/tpm/tpm_ibmvtpm.h
>>>> @@ -41,7 +41,8 @@ struct ibmvtpm_dev {
>>>> wait_queue_head_t wq;
>>>> u16 res_len;
>>>> u32 vtpm_version;
>>>> - u8 tpm_processing_cmd;
>>>> + u8 tpm_status;
>>>> +#define TPM_STATUS_BUSY (1 << 0) /* vtpm is processing a command */
>>> Declare this already in the fix, and just leave the rename here.
>> You mean the fix patch does not use 'true' anymore but uses the
>> TPM_STATUS_BUSY flag already but the name is still tpm_processing_cmd? And
>> literally only the renaming of this field is done in the 2nd patch?
> I can fixup these patches, and use '1', instead of true. No need to send
> new ones.
>
> Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
THANKS!
Stefan
>
> /Jarkko
^ permalink raw reply
* Re: [PATCH v2] fscrypt: support trusted keys
From: Mimi Zohar @ 2021-08-11 11:34 UTC (permalink / raw)
To: Jarkko Sakkinen, Eric Biggers
Cc: Ahmad Fatoum, Theodore Y. Ts'o, Jaegeuk Kim, kernel,
James Morris, Serge E. Hallyn, James Bottomley, Sumit Garg,
David Howells, linux-fscrypt, linux-crypto, linux-integrity,
linux-security-module, keyrings, linux-kernel
In-Reply-To: <20210811001743.ofzkwdwa6rcjsf4d@kernel.org>
On Wed, 2021-08-11 at 03:17 +0300, Jarkko Sakkinen wrote:
> On Tue, Aug 10, 2021 at 02:27:24PM -0700, Eric Biggers wrote:
> > On Wed, Aug 11, 2021 at 12:21:40AM +0300, Jarkko Sakkinen wrote:
> > > On Tue, Aug 10, 2021 at 11:46:49AM -0700, Eric Biggers wrote:
> > > > On Tue, Aug 10, 2021 at 09:06:36PM +0300, Jarkko Sakkinen wrote:
> > > > > > >
> > > > > > > I don't think this is right, or at least it does not follow the pattern
> > > > > > > in [*]. I.e. you should rather use trusted key to seal your fscrypt key.
> > > > > >
> > > > > > What's the benefit of the extra layer of indirection over just using a "trusted"
> > > > > > key directly? The use case for "encrypted" keys is not at all clear to me.
> > > > >
> > > > > Because it is more robust to be able to use small amount of trusted keys,
> > > > > which are not entirely software based.
> > > > >
> > > > > And since it's also a pattern on existing kernel features utilizing trusted
> > > > > keys, the pressure here to explain why break the pattern, should be on the
> > > > > side of the one who breaks it.
> > > >
> > > > This is a new feature, so it's on the person proposing the feature to explain
> > > > why it's useful. The purpose of "encrypted" keys is not at all clear, and the
> > > > documentation for them is heavily misleading. E.g.:
> > > >
> > > > "user space sees, stores, and loads only encrypted blobs"
> > > > (Not necessarily true, as I've explained previously.)
> > > >
> > > > "Encrypted keys do not depend on a trust source" ... "The main disadvantage
> > > > of encrypted keys is that if they are not rooted in a trusted key"
> > > > (Not necessarily true, and in fact it seems they're only useful when they
> > > > *do* depend on a trust source. At least that's the use case that is being
> > > > proposed here, IIUC.)
> > > >
> > > > I do see a possible use for the layer of indirection that "encrypted" keys are,
> > > > which is that it would reduce the overhead of having lots of keys be directly
> > > > encrypted by the TPM/TEE/CAAM. Is this the use case? If so, it needs to be
> > > > explained.
> > >
> > > If trusted keys are used directly, it's an introduction of a bottleneck.
> > > If they are used indirectly, you can still choose to have one trusted
> > > key per fscrypt key.
> > >
> > > Thus, it's obviously a bad idea to use them directly.
> >
> > So actually explain that in the documentation. It's not obvious at all. And
> > does this imply that the support for trusted keys in dm-crypt is a mistake?
>
> Looking at dm-crypt implementation, you can choose to use 'encrypted' key
> type, which you can seal with a trusted key.
>
> Note: I have not been involved when the feature was added to dm-crypt.
At least for TPM 1.2, "trusted" keys may be sealed to a PCR and then
extended to prevent subsequent usage. For example, in the initramfs
all of the "encrypted" keys could be decrypted by a single "trusted"
key, before extending the PCR.
Mimi
^ permalink raw reply
* Re: [PATCH 3/4] crypto: caam - add in-kernel interface for blob generator
From: David Gstir @ 2021-08-11 10:43 UTC (permalink / raw)
To: Ahmad Fatoum
Cc: Horia Geantă, Aymen Sghaier, Herbert Xu, David S. Miller,
kernel, James Bottomley, Jarkko Sakkinen, Mimi Zohar,
David Howells, James Morris, Eric Biggers, Serge E. Hallyn,
Udit Agarwal, Jan Luebbe, Richard Weinberger, Franck LENORMAND,
Sumit Garg, linux-integrity, keyrings, linux-crypto, linux-kernel,
linux-security-module
In-Reply-To: <7cc83edd-dc39-ee7e-d18c-30b2492247ea@pengutronix.de>
Hi Ahmad,
> On 11.08.2021, at 12:22, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
>> Since you already assert that MAX_BLOB_SIZE <= CAAM_BLOB_MAX_LEN
>> in security/keys/trusted-keys/trusted_caam.c, this will never
>> be an issue for CAAM-based trusted-keys though.
> I omitted checks in code, which are verified at compile-time.
> Would you prefer a runtime check to be added as well?
I’d say the compile-time check suffices, unless this is intended
to be used outside of trusted-keys. But I don’t think this is very likely…
Cheers,
David
^ permalink raw reply
* Re: [PATCH 3/4] crypto: caam - add in-kernel interface for blob generator
From: Ahmad Fatoum @ 2021-08-11 10:22 UTC (permalink / raw)
To: David Gstir
Cc: Horia Geantă, Aymen Sghaier, Herbert Xu, David S. Miller,
kernel, James Bottomley, Jarkko Sakkinen, Mimi Zohar,
David Howells, James Morris, Eric Biggers, Serge E. Hallyn,
Udit Agarwal, Jan Luebbe, Richard Weinberger, Franck LENORMAND,
Sumit Garg, linux-integrity, keyrings, linux-crypto, linux-kernel,
linux-security-module
In-Reply-To: <796E18E6-1329-40D6-B12F-4CE6C90DD988@sigma-star.at>
On 10.08.21 13:29, David Gstir wrote:
> Hi Ahmad,
>
>> On 21.07.2021, at 18:48, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
>
> [...]
>
>> diff --git a/drivers/crypto/caam/blob_gen.c b/drivers/crypto/caam/blob_gen.c
>> new file mode 100644
>> index 000000000000..513d3f90e438
>> --- /dev/null
>> +++ b/drivers/crypto/caam/blob_gen.c
>> @@ -0,0 +1,230 @@
>
> [...]
>
>> +
>> +int caam_encap_blob(struct caam_blob_priv *priv, const char *keymod,
>> + void *input, void *output, size_t length)
>> +{
>> + u32 *desc;
>> + struct device *jrdev = &priv->jrdev;
>> + dma_addr_t dma_in, dma_out;
>> + struct caam_blob_job_result testres;
>> + size_t keymod_len = strlen(keymod);
>> + int ret;
>> +
>> + if (length <= CAAM_BLOB_OVERHEAD || keymod_len > CAAM_BLOB_KEYMOD_LENGTH)
>
> The docs for this function mention the length <= CAAM_BLOB_MAX_LEN
> restriction. This is not checked here. Is this intended?
Yes.
> Since you already assert that MAX_BLOB_SIZE <= CAAM_BLOB_MAX_LEN
> in security/keys/trusted-keys/trusted_caam.c, this will never
> be an issue for CAAM-based trusted-keys though.
I omitted checks in code, which are verified at compile-time.
Would you prefer a runtime check to be added as well?
>> + return -EINVAL;
>> +
>> + desc = caam_blob_alloc_desc(keymod_len);
>> + if (!desc) {
>> + dev_err(jrdev, "unable to allocate desc\n");
>> + return -ENOMEM;
>> + }
>> +
>
> [...]
>
>> diff --git a/include/soc/fsl/caam-blob.h b/include/soc/fsl/caam-blob.h
>> new file mode 100644
>> index 000000000000..aebbc9335f64
>> --- /dev/null
>> +++ b/include/soc/fsl/caam-blob.h
>> @@ -0,0 +1,56 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (C) 2020 Pengutronix, Ahmad Fatoum <kernel@pengutronix.de>
>> + */
>> +
>> +#ifndef __CAAM_BLOB_GEN
>> +#define __CAAM_BLOB_GEN
>> +
>> +#include <linux/types.h>
>> +
>> +#define CAAM_BLOB_KEYMOD_LENGTH 16
>> +#define CAAM_BLOB_OVERHEAD (32 + 16)
>> +#define CAAM_BLOB_MAX_LEN 4096
>> +
>> +struct caam_blob_priv;
>> +
>> +/** caam_blob_gen_init - initialize blob generation
>> + *
>> + * returns either pointer to new caam_blob_priv instance
>> + * or error pointer
>> + */
>> +struct caam_blob_priv *caam_blob_gen_init(void);
>> +
>> +/** caam_blob_gen_init - free blob generation resources
>
> s/init/exit/
Oh, thanks for catching.
>> + *
>> + * @priv: instance returned by caam_blob_gen_init
>> + */
>> +void caam_blob_gen_exit(struct caam_blob_priv *priv);
>
>
> Except these minor things, I noticed no issues with this whole series:
>
> Reviewed-by: David Gstir <david@sigma-star.at>
Thanks! Will include it with the next iteration.
Cheers,
Ahmad
>
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: Documenting the requirement of CAP_SETFCAP to map UID 0
From: Michael Kerrisk (man-pages) @ 2021-08-11 10:10 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: mtk.manpages, linux-security-module, lkml, Alejandro Colomar,
Kir Kolyshkin, linux-man
In-Reply-To: <20210810235838.GA4561@mail.hallyn.com>
Hi Serge
On 8/11/21 1:58 AM, Serge E. Hallyn wrote:
> On Sun, Aug 08, 2021 at 11:09:30AM +0200, Michael Kerrisk (man-pages) wrote:
>> Hello Serge,
>>
Hello Serge,
>> Your commit:
>>
>> [[
>> commit db2e718a47984b9d71ed890eb2ea36ecf150de18
>> Author: Serge E. Hallyn <serge@hallyn.com>
>> Date: Tue Apr 20 08:43:34 2021 -0500
>>
>> capabilities: require CAP_SETFCAP to map uid 0
>> ]]
>>
>> added a new requirement when updating a UID map a user namespace
>> with a value of '0 0 *'.
>>
>> Kir sent a patch to briefly document this change, but I think much more
>> should be written. I've attempted to do so. Could you tell me whether the
>> following text (to be added in user_namespaces(7)) is accurate please:
>
> Sorry for the delay - this did not go into my main mailbox.
>
> The text looks good. Thanks!
Thanks for checking it!
Cheers,
Michael
>> [[
>> In order for a process to write to the /proc/[pid]/uid_map
>> (/proc/[pid]/gid_map) file, all of the following requirements must
>> be met:
>>
>> [...]
>>
>> 4. If updating /proc/[pid]/uid_map to create a mapping that maps
>> UID 0 in the parent namespace, then one of the following must
>> be true:
>>
>> * if writing process is in the parent user namespace, then it
>> must have the CAP_SETFCAP capability in that user namespace;
>> or
>>
>> * if the writing process is in the child user namespace, then
>> the process that created the user namespace must have had
>> the CAP_SETFCAP capability when the namespace was created.
>>
>> This rule has been in place since Linux 5.12. It eliminates an
>> earlier security bug whereby a UID 0 process that lacks the
>> CAP_SETFCAP capability, which is needed to create a binary with
>> namespaced file capabilities (as described in capabilities(7)),
>> could nevertheless create such a binary, by the following
>> steps:
>>
>> * Create a new user namespace with the identity mapping (i.e.,
>> UID 0 in the new user namespace maps to UID 0 in the parent
>> namespace), so that UID 0 in both namespaces is equivalent
>> to the same root user ID.
>>
>> * Since the child process has the CAP_SETFCAP capability, it
>> could create a binary with namespaced file capabilities that
>> would then be effective in the parent user namespace (be‐
>> cause the root user IDs are the same in the two namespaces).
>>
>> [...]
>> ]]
>>
>> Thanks,
>>
>> Michael
>>
>> --
>> Michael Kerrisk
>> Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
>> Linux/UNIX System Programming Training: http://man7.org/training/
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* Re: [PATCH v4 2/2] tpm: ibmvtpm: Rename tpm_process_cmd to tpm_status and define flag
From: Jarkko Sakkinen @ 2021-08-11 2:10 UTC (permalink / raw)
To: Stefan Berger
Cc: Stefan Berger, nasastry, linux-integrity, linux-security-module,
linux-kernel, Nayna Jain, George Wilson
In-Reply-To: <86f6a6c8-87cc-a397-35b3-a30220f12aed@linux.ibm.com>
On Tue, Aug 10, 2021 at 09:50:55PM -0400, Stefan Berger wrote:
>
> On 8/10/21 1:58 PM, Jarkko Sakkinen wrote:
> > On Mon, Aug 09, 2021 at 03:21:59PM -0400, Stefan Berger wrote:
> > > From: Stefan Berger <stefanb@linux.ibm.com>
> > >
> > > Rename the field tpm_processing_cmd to tpm_status in ibmvtpm_dev and set
> > > the TPM_STATUS_BUSY flag while the vTPM is busy processing a command.
> > >
> > >
> > > default:
> > > diff --git a/drivers/char/tpm/tpm_ibmvtpm.h b/drivers/char/tpm/tpm_ibmvtpm.h
> > > index 51198b137461..252f1cccdfc5 100644
> > > --- a/drivers/char/tpm/tpm_ibmvtpm.h
> > > +++ b/drivers/char/tpm/tpm_ibmvtpm.h
> > > @@ -41,7 +41,8 @@ struct ibmvtpm_dev {
> > > wait_queue_head_t wq;
> > > u16 res_len;
> > > u32 vtpm_version;
> > > - u8 tpm_processing_cmd;
> > > + u8 tpm_status;
> > > +#define TPM_STATUS_BUSY (1 << 0) /* vtpm is processing a command */
> > Declare this already in the fix, and just leave the rename here.
>
> You mean the fix patch does not use 'true' anymore but uses the
> TPM_STATUS_BUSY flag already but the name is still tpm_processing_cmd? And
> literally only the renaming of this field is done in the 2nd patch?
I can fixup these patches, and use '1', instead of true. No need to send
new ones.
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
/Jarkko
^ permalink raw reply
* Re: [PATCH v4 2/2] tpm: ibmvtpm: Rename tpm_process_cmd to tpm_status and define flag
From: Stefan Berger @ 2021-08-11 1:50 UTC (permalink / raw)
To: Jarkko Sakkinen, Stefan Berger
Cc: nasastry, linux-integrity, linux-security-module, linux-kernel,
Nayna Jain, George Wilson
In-Reply-To: <20210810175855.fixtw5jks4gbmkua@kernel.org>
On 8/10/21 1:58 PM, Jarkko Sakkinen wrote:
> On Mon, Aug 09, 2021 at 03:21:59PM -0400, Stefan Berger wrote:
>> From: Stefan Berger <stefanb@linux.ibm.com>
>>
>> Rename the field tpm_processing_cmd to tpm_status in ibmvtpm_dev and set
>> the TPM_STATUS_BUSY flag while the vTPM is busy processing a command.
>>
>>
>> default:
>> diff --git a/drivers/char/tpm/tpm_ibmvtpm.h b/drivers/char/tpm/tpm_ibmvtpm.h
>> index 51198b137461..252f1cccdfc5 100644
>> --- a/drivers/char/tpm/tpm_ibmvtpm.h
>> +++ b/drivers/char/tpm/tpm_ibmvtpm.h
>> @@ -41,7 +41,8 @@ struct ibmvtpm_dev {
>> wait_queue_head_t wq;
>> u16 res_len;
>> u32 vtpm_version;
>> - u8 tpm_processing_cmd;
>> + u8 tpm_status;
>> +#define TPM_STATUS_BUSY (1 << 0) /* vtpm is processing a command */
> Declare this already in the fix, and just leave the rename here.
You mean the fix patch does not use 'true' anymore but uses the
TPM_STATUS_BUSY flag already but the name is still tpm_processing_cmd?
And literally only the renaming of this field is done in the 2nd patch?
Stefan
>
>> };
>>
>> #define CRQ_RES_BUF_SIZE PAGE_SIZE
>> --
>> 2.31.1
>>
>>
> Otherwise, these look fine.
>
> /Jarkko
^ permalink raw reply
* Re: [PATCH v2] fscrypt: support trusted keys
From: Jarkko Sakkinen @ 2021-08-11 0:17 UTC (permalink / raw)
To: Eric Biggers
Cc: Ahmad Fatoum, Theodore Y. Ts'o, Jaegeuk Kim, kernel,
James Morris, Serge E. Hallyn, James Bottomley, Mimi Zohar,
Sumit Garg, David Howells, linux-fscrypt, linux-crypto,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <YRLvPJehAeMiYb2Z@gmail.com>
On Tue, Aug 10, 2021 at 02:27:24PM -0700, Eric Biggers wrote:
> On Wed, Aug 11, 2021 at 12:21:40AM +0300, Jarkko Sakkinen wrote:
> > On Tue, Aug 10, 2021 at 11:46:49AM -0700, Eric Biggers wrote:
> > > On Tue, Aug 10, 2021 at 09:06:36PM +0300, Jarkko Sakkinen wrote:
> > > > > >
> > > > > > I don't think this is right, or at least it does not follow the pattern
> > > > > > in [*]. I.e. you should rather use trusted key to seal your fscrypt key.
> > > > >
> > > > > What's the benefit of the extra layer of indirection over just using a "trusted"
> > > > > key directly? The use case for "encrypted" keys is not at all clear to me.
> > > >
> > > > Because it is more robust to be able to use small amount of trusted keys,
> > > > which are not entirely software based.
> > > >
> > > > And since it's also a pattern on existing kernel features utilizing trusted
> > > > keys, the pressure here to explain why break the pattern, should be on the
> > > > side of the one who breaks it.
> > >
> > > This is a new feature, so it's on the person proposing the feature to explain
> > > why it's useful. The purpose of "encrypted" keys is not at all clear, and the
> > > documentation for them is heavily misleading. E.g.:
> > >
> > > "user space sees, stores, and loads only encrypted blobs"
> > > (Not necessarily true, as I've explained previously.)
> > >
> > > "Encrypted keys do not depend on a trust source" ... "The main disadvantage
> > > of encrypted keys is that if they are not rooted in a trusted key"
> > > (Not necessarily true, and in fact it seems they're only useful when they
> > > *do* depend on a trust source. At least that's the use case that is being
> > > proposed here, IIUC.)
> > >
> > > I do see a possible use for the layer of indirection that "encrypted" keys are,
> > > which is that it would reduce the overhead of having lots of keys be directly
> > > encrypted by the TPM/TEE/CAAM. Is this the use case? If so, it needs to be
> > > explained.
> >
> > If trusted keys are used directly, it's an introduction of a bottleneck.
> > If they are used indirectly, you can still choose to have one trusted
> > key per fscrypt key.
> >
> > Thus, it's obviously a bad idea to use them directly.
> >
>
> So actually explain that in the documentation. It's not obvious at all. And
> does this imply that the support for trusted keys in dm-crypt is a mistake?
Looking at dm-crypt implementation, you can choose to use 'encrypted' key
type, which you can seal with a trusted key.
Note: I have not been involved when the feature was added to dm-crypt.
/Jarkko
^ permalink raw reply
* Re: Documenting the requirement of CAP_SETFCAP to map UID 0
From: Serge E. Hallyn @ 2021-08-10 23:58 UTC (permalink / raw)
To: Michael Kerrisk (man-pages)
Cc: Serge E. Hallyn, linux-security-module, lkml, Alejandro Colomar,
Kir Kolyshkin, linux-man
In-Reply-To: <14cbab6f-19f6-a28c-05d8-453ecca62180@gmail.com>
On Sun, Aug 08, 2021 at 11:09:30AM +0200, Michael Kerrisk (man-pages) wrote:
> Hello Serge,
>
> Your commit:
>
> [[
> commit db2e718a47984b9d71ed890eb2ea36ecf150de18
> Author: Serge E. Hallyn <serge@hallyn.com>
> Date: Tue Apr 20 08:43:34 2021 -0500
>
> capabilities: require CAP_SETFCAP to map uid 0
> ]]
>
> added a new requirement when updating a UID map a user namespace
> with a value of '0 0 *'.
>
> Kir sent a patch to briefly document this change, but I think much more
> should be written. I've attempted to do so. Could you tell me whether the
> following text (to be added in user_namespaces(7)) is accurate please:
Sorry for the delay - this did not go into my main mailbox.
The text looks good. Thanks!
> [[
> In order for a process to write to the /proc/[pid]/uid_map
> (/proc/[pid]/gid_map) file, all of the following requirements must
> be met:
>
> [...]
>
> 4. If updating /proc/[pid]/uid_map to create a mapping that maps
> UID 0 in the parent namespace, then one of the following must
> be true:
>
> * if writing process is in the parent user namespace, then it
> must have the CAP_SETFCAP capability in that user namespace;
> or
>
> * if the writing process is in the child user namespace, then
> the process that created the user namespace must have had
> the CAP_SETFCAP capability when the namespace was created.
>
> This rule has been in place since Linux 5.12. It eliminates an
> earlier security bug whereby a UID 0 process that lacks the
> CAP_SETFCAP capability, which is needed to create a binary with
> namespaced file capabilities (as described in capabilities(7)),
> could nevertheless create such a binary, by the following
> steps:
>
> * Create a new user namespace with the identity mapping (i.e.,
> UID 0 in the new user namespace maps to UID 0 in the parent
> namespace), so that UID 0 in both namespaces is equivalent
> to the same root user ID.
>
> * Since the child process has the CAP_SETFCAP capability, it
> could create a binary with namespaced file capabilities that
> would then be effective in the parent user namespace (be‐
> cause the root user IDs are the same in the two namespaces).
>
> [...]
> ]]
>
> Thanks,
>
> Michael
>
> --
> Michael Kerrisk
> Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
> Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* Re: [PATCH v2] fscrypt: support trusted keys
From: Eric Biggers @ 2021-08-10 21:27 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Ahmad Fatoum, Theodore Y. Ts'o, Jaegeuk Kim, kernel,
James Morris, Serge E. Hallyn, James Bottomley, Mimi Zohar,
Sumit Garg, David Howells, linux-fscrypt, linux-crypto,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <20210810212140.sdq5dq2wy5uaj7h7@kernel.org>
On Wed, Aug 11, 2021 at 12:21:40AM +0300, Jarkko Sakkinen wrote:
> On Tue, Aug 10, 2021 at 11:46:49AM -0700, Eric Biggers wrote:
> > On Tue, Aug 10, 2021 at 09:06:36PM +0300, Jarkko Sakkinen wrote:
> > > > >
> > > > > I don't think this is right, or at least it does not follow the pattern
> > > > > in [*]. I.e. you should rather use trusted key to seal your fscrypt key.
> > > >
> > > > What's the benefit of the extra layer of indirection over just using a "trusted"
> > > > key directly? The use case for "encrypted" keys is not at all clear to me.
> > >
> > > Because it is more robust to be able to use small amount of trusted keys,
> > > which are not entirely software based.
> > >
> > > And since it's also a pattern on existing kernel features utilizing trusted
> > > keys, the pressure here to explain why break the pattern, should be on the
> > > side of the one who breaks it.
> >
> > This is a new feature, so it's on the person proposing the feature to explain
> > why it's useful. The purpose of "encrypted" keys is not at all clear, and the
> > documentation for them is heavily misleading. E.g.:
> >
> > "user space sees, stores, and loads only encrypted blobs"
> > (Not necessarily true, as I've explained previously.)
> >
> > "Encrypted keys do not depend on a trust source" ... "The main disadvantage
> > of encrypted keys is that if they are not rooted in a trusted key"
> > (Not necessarily true, and in fact it seems they're only useful when they
> > *do* depend on a trust source. At least that's the use case that is being
> > proposed here, IIUC.)
> >
> > I do see a possible use for the layer of indirection that "encrypted" keys are,
> > which is that it would reduce the overhead of having lots of keys be directly
> > encrypted by the TPM/TEE/CAAM. Is this the use case? If so, it needs to be
> > explained.
>
> If trusted keys are used directly, it's an introduction of a bottleneck.
> If they are used indirectly, you can still choose to have one trusted
> key per fscrypt key.
>
> Thus, it's obviously a bad idea to use them directly.
>
So actually explain that in the documentation. It's not obvious at all. And
does this imply that the support for trusted keys in dm-crypt is a mistake?
- Eric
^ permalink raw reply
* Re: [PATCH v2] fscrypt: support trusted keys
From: Jarkko Sakkinen @ 2021-08-10 21:21 UTC (permalink / raw)
To: Eric Biggers
Cc: Ahmad Fatoum, Theodore Y. Ts'o, Jaegeuk Kim, kernel,
James Morris, Serge E. Hallyn, James Bottomley, Mimi Zohar,
Sumit Garg, David Howells, linux-fscrypt, linux-crypto,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <YRLJmaafp941uOdA@gmail.com>
On Tue, Aug 10, 2021 at 11:46:49AM -0700, Eric Biggers wrote:
> On Tue, Aug 10, 2021 at 09:06:36PM +0300, Jarkko Sakkinen wrote:
> > > >
> > > > I don't think this is right, or at least it does not follow the pattern
> > > > in [*]. I.e. you should rather use trusted key to seal your fscrypt key.
> > >
> > > What's the benefit of the extra layer of indirection over just using a "trusted"
> > > key directly? The use case for "encrypted" keys is not at all clear to me.
> >
> > Because it is more robust to be able to use small amount of trusted keys,
> > which are not entirely software based.
> >
> > And since it's also a pattern on existing kernel features utilizing trusted
> > keys, the pressure here to explain why break the pattern, should be on the
> > side of the one who breaks it.
>
> This is a new feature, so it's on the person proposing the feature to explain
> why it's useful. The purpose of "encrypted" keys is not at all clear, and the
> documentation for them is heavily misleading. E.g.:
>
> "user space sees, stores, and loads only encrypted blobs"
> (Not necessarily true, as I've explained previously.)
>
> "Encrypted keys do not depend on a trust source" ... "The main disadvantage
> of encrypted keys is that if they are not rooted in a trusted key"
> (Not necessarily true, and in fact it seems they're only useful when they
> *do* depend on a trust source. At least that's the use case that is being
> proposed here, IIUC.)
>
> I do see a possible use for the layer of indirection that "encrypted" keys are,
> which is that it would reduce the overhead of having lots of keys be directly
> encrypted by the TPM/TEE/CAAM. Is this the use case? If so, it needs to be
> explained.
If trusted keys are used directly, it's an introduction of a bottleneck.
If they are used indirectly, you can still choose to have one trusted
key per fscrypt key.
Thus, it's obviously a bad idea to use them directly.
/Jarkko
^ permalink raw reply
* Re: [PATCH v2] fscrypt: support trusted keys
From: Eric Biggers @ 2021-08-10 18:46 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Ahmad Fatoum, Theodore Y. Ts'o, Jaegeuk Kim, kernel,
James Morris, Serge E. Hallyn, James Bottomley, Mimi Zohar,
Sumit Garg, David Howells, linux-fscrypt, linux-crypto,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <20210810180636.vqwaeftv7alsodgn@kernel.org>
On Tue, Aug 10, 2021 at 09:06:36PM +0300, Jarkko Sakkinen wrote:
> > >
> > > I don't think this is right, or at least it does not follow the pattern
> > > in [*]. I.e. you should rather use trusted key to seal your fscrypt key.
> >
> > What's the benefit of the extra layer of indirection over just using a "trusted"
> > key directly? The use case for "encrypted" keys is not at all clear to me.
>
> Because it is more robust to be able to use small amount of trusted keys,
> which are not entirely software based.
>
> And since it's also a pattern on existing kernel features utilizing trusted
> keys, the pressure here to explain why break the pattern, should be on the
> side of the one who breaks it.
This is a new feature, so it's on the person proposing the feature to explain
why it's useful. The purpose of "encrypted" keys is not at all clear, and the
documentation for them is heavily misleading. E.g.:
"user space sees, stores, and loads only encrypted blobs"
(Not necessarily true, as I've explained previously.)
"Encrypted keys do not depend on a trust source" ... "The main disadvantage
of encrypted keys is that if they are not rooted in a trusted key"
(Not necessarily true, and in fact it seems they're only useful when they
*do* depend on a trust source. At least that's the use case that is being
proposed here, IIUC.)
I do see a possible use for the layer of indirection that "encrypted" keys are,
which is that it would reduce the overhead of having lots of keys be directly
encrypted by the TPM/TEE/CAAM. Is this the use case? If so, it needs to be
explained.
- Eric
^ permalink raw reply
* Re: [PATCH v2] fscrypt: support trusted keys
From: Jarkko Sakkinen @ 2021-08-10 18:06 UTC (permalink / raw)
To: Eric Biggers
Cc: Ahmad Fatoum, Theodore Y. Ts'o, Jaegeuk Kim, kernel,
James Morris, Serge E. Hallyn, James Bottomley, Mimi Zohar,
Sumit Garg, David Howells, linux-fscrypt, linux-crypto,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <YRGVcaquAJiuc8bp@gmail.com>
On Mon, Aug 09, 2021 at 01:52:01PM -0700, Eric Biggers wrote:
> On Mon, Aug 09, 2021 at 12:44:08PM +0300, Jarkko Sakkinen wrote:
> > > @@ -577,28 +578,44 @@ static int get_keyring_key(u32 key_id, u32 type,
> > > key_ref_t ref;
> > > struct key *key;
> > > const struct fscrypt_provisioning_key_payload *payload;
> > > - int err;
> > > + int err = 0;
> > >
> > > ref = lookup_user_key(key_id, 0, KEY_NEED_SEARCH);
> > > if (IS_ERR(ref))
> > > return PTR_ERR(ref);
> > > key = key_ref_to_ptr(ref);
> > >
> > > - if (key->type != &key_type_fscrypt_provisioning)
> > > - goto bad_key;
> > > - payload = key->payload.data[0];
> > > + if (key->type == &key_type_fscrypt_provisioning) {
> >
> > Why does fscrypt have own key type, and does not extend 'encrypted' with a
> > new format [*]?
>
> Are you referring to the "fscrypt-provisioning" key type? That is an existing
> feature (which in most cases isn't used, but there is a use case that requires
> it), not something being added by this patch. We just needed a key type where
> userspace can add a raw key to the kernel and not be able to read it back (so
> like the "logon" key type), but also have the kernel enforce that that key is
> only used for fscrypt with a particular KDF version, and not with other random
> kernel features. The "encrypted" key type wouldn't have worked for this at all;
> it's a totally different thing.
>
> > > + } else if (IS_REACHABLE(CONFIG_TRUSTED_KEYS) && key->type == &key_type_trusted) {
> > > + struct trusted_key_payload *tkp;
> > > +
> > > + /* avoid reseal changing payload while we memcpy key */
> > > + down_read(&key->sem);
> > > + tkp = key->payload.data[0];
> > > + if (!tkp || tkp->key_len < FSCRYPT_MIN_KEY_SIZE ||
> > > + tkp->key_len > FSCRYPT_MAX_KEY_SIZE) {
> > > + up_read(&key->sem);
> > > + err = -EINVAL;
> > > + goto out_put;
> > > + }
> > > +
> > > + secret->size = tkp->key_len;
> > > + memcpy(secret->raw, tkp->key, secret->size);
> > > + up_read(&key->sem);
> > > + } else {
> >
> >
> > I don't think this is right, or at least it does not follow the pattern
> > in [*]. I.e. you should rather use trusted key to seal your fscrypt key.
>
> What's the benefit of the extra layer of indirection over just using a "trusted"
> key directly? The use case for "encrypted" keys is not at all clear to me.
Because it is more robust to be able to use small amount of trusted keys,
which are not entirely software based.
And since it's also a pattern on existing kernel features utilizing trusted
keys, the pressure here to explain why break the pattern, should be on the
side of the one who breaks it.
/Jarkko
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox