| Message ID | 20220808144900.125242-1-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: add a new warn_on_audited debug flag to selinuxfs | expand |
On Mon, Aug 8, 2022 at 10:49 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > When debugging SELinux denials, it is often helpful to know which part > of kernel code triggered the denial. Thus, this patch adds a new > /sys/fs/selinux/warn_on_audited flag that, when set to 1, will cause any > audited AVC event to log a WARNING to the kernel console, which > naturally comes with a kernel stack trace. > > While the same can be achieved via the "avc:selinux_audited" kernel > tracepoint and the perf tool, that approach has several practical > disadvantages: > 1. It requires perf to be installed on the machine. > 2. It requires kernel debug symbols to be available when decoding the > stack trace. > 3. It requires a perf process to be running in the background. > 4. The stack traces can only be obtained at the end, after the perf > process is terminated, not live during the capture. (Though this may > be solved by writing a custom tool on top of libtraceevent.) > > Thus, providing a simple native knob for this in selinuxfs is still > valuable. > > The warn_on_audited flag is always set to 0 on boot and is expected to > be set to 1 only temporarily by system administrator in order to debug > SELinux denials. It is not intended to be used on production systems. > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > security/selinux/avc.c | 6 +++ > security/selinux/ima.c | 11 +++++- > security/selinux/include/security.h | 11 ++++++ > security/selinux/selinuxfs.c | 61 +++++++++++++++++++++++++++++ > 4 files changed, 88 insertions(+), 1 deletion(-) I'm sorry, but I'm not going to merge this. At least not now. In general I don't like using WARN/WARN_ON/etc. for this; I believe their use should be limited for rather serious kernel issues and not as a developer's debugging tool. I also don't like duplicating the tracepoint functionality. I understand there are hurdles to using perf on a system, but I would much rather see work go into fixing that than duplicating its functionality
On Tue, Aug 16, 2022 at 5:31 AM Paul Moore <paul@paul-moore.com> wrote: > On Mon, Aug 8, 2022 at 10:49 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > When debugging SELinux denials, it is often helpful to know which part > > of kernel code triggered the denial. Thus, this patch adds a new > > /sys/fs/selinux/warn_on_audited flag that, when set to 1, will cause any > > audited AVC event to log a WARNING to the kernel console, which > > naturally comes with a kernel stack trace. > > > > While the same can be achieved via the "avc:selinux_audited" kernel > > tracepoint and the perf tool, that approach has several practical > > disadvantages: > > 1. It requires perf to be installed on the machine. > > 2. It requires kernel debug symbols to be available when decoding the > > stack trace. > > 3. It requires a perf process to be running in the background. > > 4. The stack traces can only be obtained at the end, after the perf > > process is terminated, not live during the capture. (Though this may > > be solved by writing a custom tool on top of libtraceevent.) > > > > Thus, providing a simple native knob for this in selinuxfs is still > > valuable. > > > > The warn_on_audited flag is always set to 0 on boot and is expected to > > be set to 1 only temporarily by system administrator in order to debug > > SELinux denials. It is not intended to be used on production systems. > > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > > --- > > security/selinux/avc.c | 6 +++ > > security/selinux/ima.c | 11 +++++- > > security/selinux/include/security.h | 11 ++++++ > > security/selinux/selinuxfs.c | 61 +++++++++++++++++++++++++++++ > > 4 files changed, 88 insertions(+), 1 deletion(-) > > I'm sorry, but I'm not going to merge this. At least not now. > > In general I don't like using WARN/WARN_ON/etc. for this; I believe > their use should be limited for rather serious kernel issues and not > as a developer's debugging tool. I also don't like duplicating the > tracepoint functionality. I understand there are hurdles to using > perf on a system, but I would much rather see work go into fixing that > than duplicating its functionality OK, I'm not going to argue against that, but I had to at least try :) -- Ondrej Mosnacek Senior Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
diff --git a/security/selinux/avc.c b/security/selinux/avc.c index 9a43af0ebd7d..0e615c9e8e79 100644 --- a/security/selinux/avc.c +++ b/security/selinux/avc.c @@ -736,6 +736,12 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a) audit_log_format(ab, " permissive=%u", sad->result ? 0 : 1); trace_selinux_audited(sad, scontext, tcontext, tclass); + + WARN(warn_on_audited_get(sad->state), + "SELinux: AV: requested=0x%x denied=0x%x audited=0x%x result=%d scontext=%s tcontext=%s tclass=%s", + sad->requested, sad->denied, sad->audited, sad->result, + scontext, tcontext, tclass); + kfree(tcontext); kfree(scontext); diff --git a/security/selinux/ima.c b/security/selinux/ima.c index a915b89d55b0..506e880040f5 100644 --- a/security/selinux/ima.c +++ b/security/selinux/ima.c @@ -26,7 +26,10 @@ static char *selinux_ima_collect_state(struct selinux_state *state) char *buf; int buf_len, len, i, rc; - buf_len = strlen("initialized=0;enforcing=0;checkreqprot=0;") + 1; + buf_len = strlen("initialized=0;" + "enforcing=0;" + "checkreqprot=0;" + "warn_on_audited=0;") + 1; len = strlen(on); for (i = 0; i < __POLICYDB_CAP_MAX; i++) @@ -54,6 +57,12 @@ static char *selinux_ima_collect_state(struct selinux_state *state) rc = strlcat(buf, checkreqprot_get(state) ? on : off, buf_len); WARN_ON(rc >= buf_len); + rc = strlcat(buf, "warn_on_audited", buf_len); + WARN_ON(rc >= buf_len); + + rc = strlcat(buf, warn_on_audited_get(state) ? on : off, buf_len); + WARN_ON(rc >= buf_len); + for (i = 0; i < __POLICYDB_CAP_MAX; i++) { rc = strlcat(buf, selinux_policycap_names[i], buf_len); WARN_ON(rc >= buf_len); diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h index 393aff41d3ef..bb1f0507edb6 100644 --- a/security/selinux/include/security.h +++ b/security/selinux/include/security.h @@ -97,6 +97,7 @@ struct selinux_state { bool enforcing; #endif bool checkreqprot; + bool warn_on_audited; bool initialized; bool policycap[__POLICYDB_CAP_MAX]; @@ -157,6 +158,16 @@ static inline void checkreqprot_set(struct selinux_state *state, bool value) WRITE_ONCE(state->checkreqprot, value); } +static inline bool warn_on_audited_get(const struct selinux_state *state) +{ + return READ_ONCE(state->warn_on_audited); +} + +static inline void warn_on_audited_set(struct selinux_state *state, bool value) +{ + WRITE_ONCE(state->warn_on_audited, value); +} + #ifdef CONFIG_SECURITY_SELINUX_DISABLE static inline bool selinux_disabled(struct selinux_state *state) { diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index 8fcdd494af27..c3774ba39cf8 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -64,6 +64,7 @@ enum sel_inos { SEL_STATUS, /* export current status using mmap() */ SEL_POLICY, /* allow userspace to read the in kernel policy */ SEL_VALIDATE_TRANS, /* compute validatetrans decision */ + SEL_WARN_ON_AUDITED, /* whether to WARN() when a denial is audited */ SEL_INO_NEXT, /* The next inode number to use */ }; @@ -857,6 +858,64 @@ static const struct file_operations sel_transition_ops = { .llseek = generic_file_llseek, }; +static ssize_t sel_read_warn_on_audited(struct file *filp, char __user *buf, + size_t count, loff_t *ppos) +{ + struct selinux_fs_info *fsi = file_inode(filp)->i_sb->s_fs_info; + char tmpbuf[TMPBUFLEN]; + ssize_t length; + + length = scnprintf(tmpbuf, TMPBUFLEN, "%u", + warn_on_audited_get(fsi->state)); + return simple_read_from_buffer(buf, count, ppos, tmpbuf, length); +} + +static ssize_t sel_write_warn_on_audited(struct file *file, const char __user *buf, + size_t count, loff_t *ppos) +{ + struct selinux_fs_info *fsi = file_inode(file)->i_sb->s_fs_info; + char *page; + ssize_t length; + unsigned int new_value; + + length = avc_has_perm(&selinux_state, + current_sid(), SECINITSID_SECURITY, + SECCLASS_SECURITY, SECURITY__SETSECPARAM, + NULL); + if (length) + return length; + + if (count >= PAGE_SIZE) + return -ENOMEM; + + /* No partial writes. */ + if (*ppos != 0) + return -EINVAL; + + page = memdup_user_nul(buf, count); + if (IS_ERR(page)) + return PTR_ERR(page); + + length = -EINVAL; + if (sscanf(page, "%u", &new_value) != 1) + goto out; + + warn_on_audited_set(fsi->state, !!new_value); + length = count; + + selinux_ima_measure_state(fsi->state); + +out: + kfree(page); + return length; +} + +static const struct file_operations sel_warn_on_audited_ops = { + .read = sel_read_warn_on_audited, + .write = sel_write_warn_on_audited, + .llseek = generic_file_llseek, +}; + /* * Remaining nodes use transaction based IO methods like nfsd/nfsctl.c */ @@ -2084,6 +2143,8 @@ static int sel_fill_super(struct super_block *sb, struct fs_context *fc) [SEL_POLICY] = {"policy", &sel_policy_ops, S_IRUGO}, [SEL_VALIDATE_TRANS] = {"validatetrans", &sel_transition_ops, S_IWUGO}, + [SEL_WARN_ON_AUDITED] = {"warn_on_audited", &sel_warn_on_audited_ops, + S_IRUGO|S_IWUSR}, /* last one */ {""} };
When debugging SELinux denials, it is often helpful to know which part of kernel code triggered the denial. Thus, this patch adds a new /sys/fs/selinux/warn_on_audited flag that, when set to 1, will cause any audited AVC event to log a WARNING to the kernel console, which naturally comes with a kernel stack trace. While the same can be achieved via the "avc:selinux_audited" kernel tracepoint and the perf tool, that approach has several practical disadvantages: 1. It requires perf to be installed on the machine. 2. It requires kernel debug symbols to be available when decoding the stack trace. 3. It requires a perf process to be running in the background. 4. The stack traces can only be obtained at the end, after the perf process is terminated, not live during the capture. (Though this may be solved by writing a custom tool on top of libtraceevent.) Thus, providing a simple native knob for this in selinuxfs is still valuable. The warn_on_audited flag is always set to 0 on boot and is expected to be set to 1 only temporarily by system administrator in order to debug SELinux denials. It is not intended to be used on production systems. Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- security/selinux/avc.c | 6 +++ security/selinux/ima.c | 11 +++++- security/selinux/include/security.h | 11 ++++++ security/selinux/selinuxfs.c | 61 +++++++++++++++++++++++++++++ 4 files changed, 88 insertions(+), 1 deletion(-)