12:32 p.m.
Remove audit_context from struct task_struct and struct audit_buffer
when CONFIG_AUDIT is enabled but CONFIG_AUDITSYSCALL is not.
Also, audit_log_name() (and supporting inode and fcaps functions) should
have been put back in auditsc.c when soft and hard link logging was
normalized since it is only used by syscall auditing.
See github issue https://github.com/linux-audit/audit-kernel/issues/105
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
Changelog:
v2:
- resolve merge conflicts from rebase on upstreamed ghak103 patch
- wrap task_struct audit_context in CONFIG_AUDITSYSCALL
include/linux/sched.h | 4 +-
kernel/audit.c | 157 +++-----------------------------------------------
kernel/audit.h | 9 ---
kernel/auditsc.c | 150 +++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 161 insertions(+), 159 deletions(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index f9788bb122c5..765119df759a 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -885,8 +885,10 @@ struct task_struct {
struct callback_head *task_works;
- struct audit_context *audit_context;
#ifdef CONFIG_AUDIT
+#ifdef CONFIG_AUDITSYSCALL
+ struct audit_context *audit_context;
+#endif
kuid_t loginuid;
unsigned int sessionid;
#endif
diff --git a/kernel/audit.c b/kernel/audit.c
index 3f3f1888cac7..15e41603fd34 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -205,7 +205,9 @@ struct audit_net {
* use simultaneously. */
struct audit_buffer {
struct sk_buff *skb; /* formatted skb ready to send */
+#ifdef CONFIG_AUDITSYSCALL
struct audit_context *ctx; /* NULL or associated context */
+#endif
gfp_t gfp_mask;
};
@@ -1696,7 +1698,9 @@ static struct audit_buffer *audit_buffer_alloc(struct audit_context
*ctx,
if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
goto err;
+#ifdef CONFIG_AUDITSYSCALL
ab->ctx = ctx;
+#endif
ab->gfp_mask = gfp_mask;
return ab;
@@ -1809,7 +1813,11 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx,
gfp_t gfp_mask,
return NULL;
}
+#ifdef CONFIG_AUDITSYSCALL
audit_get_stamp(ab->ctx, &t, &serial);
+#else
+ audit_get_stamp(NULL, &t, &serial);
+#endif
audit_log_format(ab, "audit(%llu.%03lu:%u): ",
(unsigned long long)t.tv_sec, t.tv_nsec/1000000, serial);
@@ -2067,155 +2075,6 @@ void audit_log_key(struct audit_buffer *ab, char *key)
audit_log_format(ab, "(null)");
}
-void audit_log_cap(struct audit_buffer *ab, char *prefix, kernel_cap_t *cap)
-{
- int i;
-
- if (cap_isclear(*cap)) {
- audit_log_format(ab, " %s=0", prefix);
- return;
- }
- audit_log_format(ab, " %s=", prefix);
- CAP_FOR_EACH_U32(i)
- audit_log_format(ab, "%08x", cap->cap[CAP_LAST_U32 - i]);
-}
-
-static void audit_log_fcaps(struct audit_buffer *ab, struct audit_names *name)
-{
- audit_log_cap(ab, "cap_fp", &name->fcap.permitted);
- audit_log_cap(ab, "cap_fi", &name->fcap.inheritable);
- audit_log_format(ab, " cap_fe=%d cap_fver=%x cap_frootid=%d",
- name->fcap.fE, name->fcap_ver,
- from_kuid(&init_user_ns, name->fcap.rootid));
-}
-
-static inline int audit_copy_fcaps(struct audit_names *name,
- const struct dentry *dentry)
-{
- struct cpu_vfs_cap_data caps;
- int rc;
-
- if (!dentry)
- return 0;
-
- rc = get_vfs_caps_from_disk(dentry, &caps);
- if (rc)
- return rc;
-
- name->fcap.permitted = caps.permitted;
- name->fcap.inheritable = caps.inheritable;
- name->fcap.fE = !!(caps.magic_etc & VFS_CAP_FLAGS_EFFECTIVE);
- name->fcap.rootid = caps.rootid;
- name->fcap_ver = (caps.magic_etc & VFS_CAP_REVISION_MASK) >>
- VFS_CAP_REVISION_SHIFT;
-
- return 0;
-}
-
-/* Copy inode data into an audit_names. */
-void audit_copy_inode(struct audit_names *name, const struct dentry *dentry,
- struct inode *inode)
-{
- name->ino = inode->i_ino;
- name->dev = inode->i_sb->s_dev;
- name->mode = inode->i_mode;
- name->uid = inode->i_uid;
- name->gid = inode->i_gid;
- name->rdev = inode->i_rdev;
- security_inode_getsecid(inode, &name->osid);
- audit_copy_fcaps(name, dentry);
-}
-
-/**
- * audit_log_name - produce AUDIT_PATH record from struct audit_names
- * @context: audit_context for the task
- * @n: audit_names structure with reportable details
- * @path: optional path to report instead of audit_names->name
- * @record_num: record number to report when handling a list of names
- * @call_panic: optional pointer to int that will be updated if secid fails
- */
-void audit_log_name(struct audit_context *context, struct audit_names *n,
- const struct path *path, int record_num, int *call_panic)
-{
- struct audit_buffer *ab;
- ab = audit_log_start(context, GFP_KERNEL, AUDIT_PATH);
- if (!ab)
- return;
-
- audit_log_format(ab, "item=%d", record_num);
-
- if (path)
- audit_log_d_path(ab, " name=", path);
- else if (n->name) {
- switch (n->name_len) {
- case AUDIT_NAME_FULL:
- /* log the full path */
- audit_log_format(ab, " name=");
- audit_log_untrustedstring(ab, n->name->name);
- break;
- case 0:
- /* name was specified as a relative path and the
- * directory component is the cwd */
- audit_log_d_path(ab, " name=", &context->pwd);
- break;
- default:
- /* log the name's directory component */
- audit_log_format(ab, " name=");
- audit_log_n_untrustedstring(ab, n->name->name,
- n->name_len);
- }
- } else
- audit_log_format(ab, " name=(null)");
-
- if (n->ino != AUDIT_INO_UNSET)
- audit_log_format(ab, " inode=%lu"
- " dev=%02x:%02x mode=%#ho"
- " ouid=%u ogid=%u rdev=%02x:%02x",
- n->ino,
- MAJOR(n->dev),
- MINOR(n->dev),
- n->mode,
- from_kuid(&init_user_ns, n->uid),
- from_kgid(&init_user_ns, n->gid),
- MAJOR(n->rdev),
- MINOR(n->rdev));
- if (n->osid != 0) {
- char *ctx = NULL;
- u32 len;
- if (security_secid_to_secctx(
- n->osid, &ctx, &len)) {
- audit_log_format(ab, " osid=%u", n->osid);
- if (call_panic)
- *call_panic = 2;
- } else {
- audit_log_format(ab, " obj=%s", ctx);
- security_release_secctx(ctx, len);
- }
- }
-
- /* log the audit_names record type */
- switch(n->type) {
- case AUDIT_TYPE_NORMAL:
- audit_log_format(ab, " nametype=NORMAL");
- break;
- case AUDIT_TYPE_PARENT:
- audit_log_format(ab, " nametype=PARENT");
- break;
- case AUDIT_TYPE_CHILD_DELETE:
- audit_log_format(ab, " nametype=DELETE");
- break;
- case AUDIT_TYPE_CHILD_CREATE:
- audit_log_format(ab, " nametype=CREATE");
- break;
- default:
- audit_log_format(ab, " nametype=UNKNOWN");
- break;
- }
-
- audit_log_fcaps(ab, n);
- audit_log_end(ab);
-}
-
int audit_log_task_context(struct audit_buffer *ab)
{
char *ctx = NULL;
diff --git a/kernel/audit.h b/kernel/audit.h
index 9acb8691ed87..82734f438ddd 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -213,15 +213,6 @@ struct audit_context {
extern void audit_log_session_info(struct audit_buffer *ab);
-extern void audit_copy_inode(struct audit_names *name,
- const struct dentry *dentry,
- struct inode *inode);
-extern void audit_log_cap(struct audit_buffer *ab, char *prefix,
- kernel_cap_t *cap);
-extern void audit_log_name(struct audit_context *context,
- struct audit_names *n, const struct path *path,
- int record_num, int *call_panic);
-
extern int auditd_test_task(struct task_struct *task);
#define AUDIT_INODE_BUCKETS 32
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index a2696ce790f9..5a56f3ec156c 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1136,6 +1136,28 @@ static void audit_log_execve_info(struct audit_context *context,
kfree(buf_head);
}
+void audit_log_cap(struct audit_buffer *ab, char *prefix, kernel_cap_t *cap)
+{
+ int i;
+
+ if (cap_isclear(*cap)) {
+ audit_log_format(ab, " %s=0", prefix);
+ return;
+ }
+ audit_log_format(ab, " %s=", prefix);
+ CAP_FOR_EACH_U32(i)
+ audit_log_format(ab, "%08x", cap->cap[CAP_LAST_U32 - i]);
+}
+
+static void audit_log_fcaps(struct audit_buffer *ab, struct audit_names *name)
+{
+ audit_log_cap(ab, "cap_fp", &name->fcap.permitted);
+ audit_log_cap(ab, "cap_fi", &name->fcap.inheritable);
+ audit_log_format(ab, " cap_fe=%d cap_fver=%x cap_frootid=%d",
+ name->fcap.fE, name->fcap_ver,
+ from_kuid(&init_user_ns, name->fcap.rootid));
+}
+
static void show_special(struct audit_context *context, int *call_panic)
{
struct audit_buffer *ab;
@@ -1258,6 +1280,97 @@ static inline int audit_proctitle_rtrim(char *proctitle, int len)
return len;
}
+/*
+ * audit_log_name - produce AUDIT_PATH record from struct audit_names
+ * @context: audit_context for the task
+ * @n: audit_names structure with reportable details
+ * @path: optional path to report instead of audit_names->name
+ * @record_num: record number to report when handling a list of names
+ * @call_panic: optional pointer to int that will be updated if secid fails
+ */
+static void audit_log_name(struct audit_context *context, struct audit_names *n,
+ const struct path *path, int record_num, int *call_panic)
+{
+ struct audit_buffer *ab;
+
+ ab = audit_log_start(context, GFP_KERNEL, AUDIT_PATH);
+ if (!ab)
+ return;
+
+ audit_log_format(ab, "item=%d", record_num);
+
+ if (path)
+ audit_log_d_path(ab, " name=", path);
+ else if (n->name) {
+ switch (n->name_len) {
+ case AUDIT_NAME_FULL:
+ /* log the full path */
+ audit_log_format(ab, " name=");
+ audit_log_untrustedstring(ab, n->name->name);
+ break;
+ case 0:
+ /* name was specified as a relative path and the
+ * directory component is the cwd
+ */
+ audit_log_d_path(ab, " name=", &context->pwd);
+ break;
+ default:
+ /* log the name's directory component */
+ audit_log_format(ab, " name=");
+ audit_log_n_untrustedstring(ab, n->name->name,
+ n->name_len);
+ }
+ } else
+ audit_log_format(ab, " name=(null)");
+
+ if (n->ino != AUDIT_INO_UNSET)
+ audit_log_format(ab, " inode=%lu dev=%02x:%02x mode=%#ho ouid=%u ogid=%u
rdev=%02x:%02x",
+ n->ino,
+ MAJOR(n->dev),
+ MINOR(n->dev),
+ n->mode,
+ from_kuid(&init_user_ns, n->uid),
+ from_kgid(&init_user_ns, n->gid),
+ MAJOR(n->rdev),
+ MINOR(n->rdev));
+ if (n->osid != 0) {
+ char *ctx = NULL;
+ u32 len;
+
+ if (security_secid_to_secctx(
+ n->osid, &ctx, &len)) {
+ audit_log_format(ab, " osid=%u", n->osid);
+ if (call_panic)
+ *call_panic = 2;
+ } else {
+ audit_log_format(ab, " obj=%s", ctx);
+ security_release_secctx(ctx, len);
+ }
+ }
+
+ /* log the audit_names record type */
+ switch (n->type) {
+ case AUDIT_TYPE_NORMAL:
+ audit_log_format(ab, " nametype=NORMAL");
+ break;
+ case AUDIT_TYPE_PARENT:
+ audit_log_format(ab, " nametype=PARENT");
+ break;
+ case AUDIT_TYPE_CHILD_DELETE:
+ audit_log_format(ab, " nametype=DELETE");
+ break;
+ case AUDIT_TYPE_CHILD_CREATE:
+ audit_log_format(ab, " nametype=CREATE");
+ break;
+ default:
+ audit_log_format(ab, " nametype=UNKNOWN");
+ break;
+ }
+
+ audit_log_fcaps(ab, n);
+ audit_log_end(ab);
+}
+
static void audit_log_proctitle(void)
{
int res;
@@ -1753,6 +1866,43 @@ void __audit_getname(struct filename *name)
get_fs_pwd(current->fs, &context->pwd);
}
+static inline int audit_copy_fcaps(struct audit_names *name,
+ const struct dentry *dentry)
+{
+ struct cpu_vfs_cap_data caps;
+ int rc;
+
+ if (!dentry)
+ return 0;
+
+ rc = get_vfs_caps_from_disk(dentry, &caps);
+ if (rc)
+ return rc;
+
+ name->fcap.permitted = caps.permitted;
+ name->fcap.inheritable = caps.inheritable;
+ name->fcap.fE = !!(caps.magic_etc & VFS_CAP_FLAGS_EFFECTIVE);
+ name->fcap.rootid = caps.rootid;
+ name->fcap_ver = (caps.magic_etc & VFS_CAP_REVISION_MASK) >>
+ VFS_CAP_REVISION_SHIFT;
+
+ return 0;
+}
+
+/* Copy inode data into an audit_names. */
+void audit_copy_inode(struct audit_names *name, const struct dentry *dentry,
+ struct inode *inode)
+{
+ name->ino = inode->i_ino;
+ name->dev = inode->i_sb->s_dev;
+ name->mode = inode->i_mode;
+ name->uid = inode->i_uid;
+ name->gid = inode->i_gid;
+ name->rdev = inode->i_rdev;
+ security_inode_getsecid(inode, &name->osid);
+ audit_copy_fcaps(name, dentry);
+}
+
/**
* __audit_inode - store the inode and device from a lookup
* @name: name being audited
--
1.8.3.1
5:07 p.m.
On Mon, Jan 28, 2019 at 1:33 PM Richard Guy Briggs <rgb(a)redhat.com> wrote:
Remove audit_context from struct task_struct and struct audit_buffer
when CONFIG_AUDIT is enabled but CONFIG_AUDITSYSCALL is not.
Also, audit_log_name() (and supporting inode and fcaps functions) should
have been put back in auditsc.c when soft and hard link logging was
normalized since it is only used by syscall auditing.
See github issue https://github.com/linux-audit/audit-kernel/issues/105
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
Changelog:
v2:
- resolve merge conflicts from rebase on upstreamed ghak103 patch
- wrap task_struct audit_context in CONFIG_AUDITSYSCALL
include/linux/sched.h | 4 +-
kernel/audit.c | 157 +++-----------------------------------------------
kernel/audit.h | 9 ---
kernel/auditsc.c | 150 +++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 161 insertions(+), 159 deletions(-)
...
diff --git a/kernel/audit.c b/kernel/audit.c
index 3f3f1888cac7..15e41603fd34 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -205,7 +205,9 @@ struct audit_net {
* use simultaneously. */
struct audit_buffer {
struct sk_buff *skb; /* formatted skb ready to send */
+#ifdef CONFIG_AUDITSYSCALL
struct audit_context *ctx; /* NULL or associated context */
+#endif
gfp_t gfp_mask;
};
@@ -1696,7 +1698,9 @@ static struct audit_buffer *audit_buffer_alloc(struct audit_context
*ctx,
if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
goto err;
+#ifdef CONFIG_AUDITSYSCALL
ab->ctx = ctx;
+#endif
I vaguely remember reading/hearing something in the past about
kmem_cache_alloc() not returning a zero'd out buffer in all cases, can
you say for certain that "ab" in this case is always going to be
zero'd out? This is an honest question.
If we can't guarantee that "ab" is zero'd out, we should manually set
ab->ctx to NULL when !CONFIG_AUDITSYSCALL.
ab->gfp_mask = gfp_mask;
return ab;
@@ -1809,7 +1813,11 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx,
gfp_t gfp_mask,
return NULL;
}
+#ifdef CONFIG_AUDITSYSCALL
audit_get_stamp(ab->ctx, &t, &serial);
+#else
+ audit_get_stamp(NULL, &t, &serial);
+#endif
If ab->ctx is NULL we don't really need this, do we?
audit_log_format(ab, "audit(%llu.%03lu:%u): ",
(unsigned long long)t.tv_sec, t.tv_nsec/1000000, serial);
--
paul moore
www.paul-moore.com
5:17 p.m.
On 2019-01-29 18:07, Paul Moore wrote:
On Mon, Jan 28, 2019 at 1:33 PM Richard Guy Briggs
<rgb(a)redhat.com> wrote:
> Remove audit_context from struct task_struct and struct audit_buffer
> when CONFIG_AUDIT is enabled but CONFIG_AUDITSYSCALL is not.
>
> Also, audit_log_name() (and supporting inode and fcaps functions) should
> have been put back in auditsc.c when soft and hard link logging was
> normalized since it is only used by syscall auditing.
>
> See github issue https://github.com/linux-audit/audit-kernel/issues/105
>
> Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
> ---
> Changelog:
> v2:
> - resolve merge conflicts from rebase on upstreamed ghak103 patch
> - wrap task_struct audit_context in CONFIG_AUDITSYSCALL
>
> include/linux/sched.h | 4 +-
> kernel/audit.c | 157 +++-----------------------------------------------
> kernel/audit.h | 9 ---
> kernel/auditsc.c | 150 +++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 161 insertions(+), 159 deletions(-)
...
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 3f3f1888cac7..15e41603fd34 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -205,7 +205,9 @@ struct audit_net {
> * use simultaneously. */
> struct audit_buffer {
> struct sk_buff *skb; /* formatted skb ready to send */
> +#ifdef CONFIG_AUDITSYSCALL
> struct audit_context *ctx; /* NULL or associated context */
> +#endif
> gfp_t gfp_mask;
> };
>
> @@ -1696,7 +1698,9 @@ static struct audit_buffer *audit_buffer_alloc(struct
audit_context *ctx,
> if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
> goto err;
>
> +#ifdef CONFIG_AUDITSYSCALL
> ab->ctx = ctx;
> +#endif
I vaguely remember reading/hearing something in the past about
kmem_cache_alloc() not returning a zero'd out buffer in all cases, can
you say for certain that "ab" in this case is always going to be
zero'd out? This is an honest question.
Ok, then maybe we should be using kmem_cache_zalloc() instead of
kmem_cache_alloc() in audit_buffer_alloc()? (as I've done in
the last patch of ghak81/first patch of ghak90)
If this is too much overhead, then we can initialize ctx = NULL;
If we can't guarantee that "ab" is zero'd out, we
should manually set
ab->ctx to NULL when !CONFIG_AUDITSYSCALL.
But ctx isn't part of struct audit_buffer when !CONFIG_AUDITSYSCALL. It
is #ifdef-ed out. What am I missing?
> ab->gfp_mask = gfp_mask;
>
> return ab;
> @@ -1809,7 +1813,11 @@ struct audit_buffer *audit_log_start(struct audit_context
*ctx, gfp_t gfp_mask,
> return NULL;
> }
>
> +#ifdef CONFIG_AUDITSYSCALL
> audit_get_stamp(ab->ctx, &t, &serial);
> +#else
> + audit_get_stamp(NULL, &t, &serial);
> +#endif
If ab->ctx is NULL we don't really need this, do we?
We do if ctx isn't part of struct audit_buffer when
!CONFIG_AUDITSYSCALL.
> audit_log_format(ab, "audit(%llu.%03lu:%u):
",
> (unsigned long long)t.tv_sec, t.tv_nsec/1000000, serial);
>
paul moore
- RGB
--
Richard Guy Briggs <rgb(a)redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
5:26 p.m.
On Tue, Jan 29, 2019 at 6:18 PM Richard Guy Briggs <rgb(a)redhat.com> wrote:
On 2019-01-29 18:07, Paul Moore wrote:
> On Mon, Jan 28, 2019 at 1:33 PM Richard Guy Briggs <rgb(a)redhat.com> wrote:
> > Remove audit_context from struct task_struct and struct audit_buffer
> > when CONFIG_AUDIT is enabled but CONFIG_AUDITSYSCALL is not.
> >
> > Also, audit_log_name() (and supporting inode and fcaps functions) should
> > have been put back in auditsc.c when soft and hard link logging was
> > normalized since it is only used by syscall auditing.
> >
> > See github issue https://github.com/linux-audit/audit-kernel/issues/105
> >
> > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
> > ---
> > Changelog:
> > v2:
> > - resolve merge conflicts from rebase on upstreamed ghak103 patch
> > - wrap task_struct audit_context in CONFIG_AUDITSYSCALL
> >
> > include/linux/sched.h | 4 +-
> > kernel/audit.c | 157
+++-----------------------------------------------
> > kernel/audit.h | 9 ---
> > kernel/auditsc.c | 150 +++++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 161 insertions(+), 159 deletions(-)
>
> ...
>
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 3f3f1888cac7..15e41603fd34 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -205,7 +205,9 @@ struct audit_net {
> > * use simultaneously. */
> > struct audit_buffer {
> > struct sk_buff *skb; /* formatted skb ready to send */
> > +#ifdef CONFIG_AUDITSYSCALL
> > struct audit_context *ctx; /* NULL or associated context */
> > +#endif
> > gfp_t gfp_mask;
> > };
> >
> > @@ -1696,7 +1698,9 @@ static struct audit_buffer *audit_buffer_alloc(struct
audit_context *ctx,
> > if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
> > goto err;
> >
> > +#ifdef CONFIG_AUDITSYSCALL
> > ab->ctx = ctx;
> > +#endif
>
> I vaguely remember reading/hearing something in the past about
> kmem_cache_alloc() not returning a zero'd out buffer in all cases, can
> you say for certain that "ab" in this case is always going to be
> zero'd out? This is an honest question.
Ok, then maybe we should be using kmem_cache_zalloc() instead of
kmem_cache_alloc() in audit_buffer_alloc()? (as I've done in
the last patch of ghak81/first patch of ghak90)
If this is too much overhead, then we can initialize ctx = NULL;
We don't need zalloc() since we're setting all the fields, although
more on this below ...
> If we can't guarantee that "ab" is zero'd out,
we should manually set
> ab->ctx to NULL when !CONFIG_AUDITSYSCALL.
But ctx isn't part of struct audit_buffer when !CONFIG_AUDITSYSCALL. It
is #ifdef-ed out. What am I missing?
You're not, I am. I saw the obvious bit where you removed it from the
task_struct, but completely glossed over the bit where you also
removed it from the audit_buffer struct. My mistake.
Once the audit container ID stuff lands we are going to need to have
the audit_context pointer in the audit_buffer regardless of the
CONFIG_AUDITSYSCALL setting, right? Assuming the answer is yes, I
think I'd just assume leave the pointer in the audit_buffer (setting
it to NULL when !CONFIG_AUDITSYSCALL) so we don't have to have those
#ifdef's in the middle of the functions (I generally like to avoid
those if possible). I think it's still worth making the changes to
task_struct, as that is the right thing to do and doesn't have the
same level of impact.
--
paul moore
www.paul-moore.com
8:54 p.m.
On 2019-01-29 18:26, Paul Moore wrote:
On Tue, Jan 29, 2019 at 6:18 PM Richard Guy Briggs
<rgb(a)redhat.com> wrote:
> On 2019-01-29 18:07, Paul Moore wrote:
> > On Mon, Jan 28, 2019 at 1:33 PM Richard Guy Briggs <rgb(a)redhat.com>
wrote:
> > > Remove audit_context from struct task_struct and struct audit_buffer
> > > when CONFIG_AUDIT is enabled but CONFIG_AUDITSYSCALL is not.
> > >
> > > Also, audit_log_name() (and supporting inode and fcaps functions) should
> > > have been put back in auditsc.c when soft and hard link logging was
> > > normalized since it is only used by syscall auditing.
> > >
> > > See github issue https://github.com/linux-audit/audit-kernel/issues/105
> > >
> > > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
> > > ---
> > > Changelog:
> > > v2:
> > > - resolve merge conflicts from rebase on upstreamed ghak103 patch
> > > - wrap task_struct audit_context in CONFIG_AUDITSYSCALL
> > >
> > > include/linux/sched.h | 4 +-
> > > kernel/audit.c | 157
+++-----------------------------------------------
> > > kernel/audit.h | 9 ---
> > > kernel/auditsc.c | 150
+++++++++++++++++++++++++++++++++++++++++++++++
> > > 4 files changed, 161 insertions(+), 159 deletions(-)
> >
> > ...
> >
> > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > index 3f3f1888cac7..15e41603fd34 100644
> > > --- a/kernel/audit.c
> > > +++ b/kernel/audit.c
> > > @@ -205,7 +205,9 @@ struct audit_net {
> > > * use simultaneously. */
> > > struct audit_buffer {
> > > struct sk_buff *skb; /* formatted skb ready to send */
> > > +#ifdef CONFIG_AUDITSYSCALL
> > > struct audit_context *ctx; /* NULL or associated context */
> > > +#endif
> > > gfp_t gfp_mask;
> > > };
> > >
> > > @@ -1696,7 +1698,9 @@ static struct audit_buffer
*audit_buffer_alloc(struct audit_context *ctx,
> > > if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
> > > goto err;
> > >
> > > +#ifdef CONFIG_AUDITSYSCALL
> > > ab->ctx = ctx;
> > > +#endif
> >
> > I vaguely remember reading/hearing something in the past about
> > kmem_cache_alloc() not returning a zero'd out buffer in all cases, can
> > you say for certain that "ab" in this case is always going to be
> > zero'd out? This is an honest question.
>
> Ok, then maybe we should be using kmem_cache_zalloc() instead of
> kmem_cache_alloc() in audit_buffer_alloc()? (as I've done in
> the last patch of ghak81/first patch of ghak90)
>
> If this is too much overhead, then we can initialize ctx = NULL;
We don't need zalloc() since we're setting all the fields, although
more on this below ...
Ok...
> > If we can't guarantee that "ab" is zero'd
out, we should manually set
> > ab->ctx to NULL when !CONFIG_AUDITSYSCALL.
>
> But ctx isn't part of struct audit_buffer when !CONFIG_AUDITSYSCALL. It
> is #ifdef-ed out. What am I missing?
You're not, I am. I saw the obvious bit where you removed it from the
task_struct, but completely glossed over the bit where you also
removed it from the audit_buffer struct. My mistake.
Once the audit container ID stuff lands we are going to need to have
the audit_context pointer in the audit_buffer regardless of the
CONFIG_AUDITSYSCALL setting, right? Assuming the answer is yes, I
think I'd just assume leave the pointer in the audit_buffer (setting
it to NULL when !CONFIG_AUDITSYSCALL) so we don't have to have those
#ifdef's in the middle of the functions (I generally like to avoid
those if possible). I think it's still worth making the changes to
task_struct, as that is the right thing to do and doesn't have the
same level of impact.
I like to avoid #ifdef compiler directives out when I can too, creating
stubs in the header file to do the job.
Why do we need an audit_context pointer in struct audit_buffer? I'll
take a stab at answering this... I was thinking it wasn't necessary,
but now I think I see what I was missing. I think the only reason is to
connect records to one event through the timestamp and serial number
when syscall is disabled. Up until now it wasn't needed unless full
syscall functionality was present, but once we have an audit container
identifier aux record we will need to join them, at minimum with a local
context for user and netfilter_pkt records.
So I have to ask, does it make sense to restructure things so that the
struct audit_buffer has a serial and ctime field so that it isn't needed
in the struct audit_context? I'm not sure if this is possible. I'll
have to go back and look at the code to see if this is the case...
paul moore
- RGB
--
Richard Guy Briggs <rgb(a)redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
9:53 p.m.
On Tue, Jan 29, 2019 at 9:54 PM Richard Guy Briggs <rgb(a)redhat.com> wrote:
On 2019-01-29 18:26, Paul Moore wrote:
> On Tue, Jan 29, 2019 at 6:18 PM Richard Guy Briggs <rgb(a)redhat.com> wrote:
> > On 2019-01-29 18:07, Paul Moore wrote:
> > > On Mon, Jan 28, 2019 at 1:33 PM Richard Guy Briggs <rgb(a)redhat.com>
wrote:
> > > > Remove audit_context from struct task_struct and struct audit_buffer
> > > > when CONFIG_AUDIT is enabled but CONFIG_AUDITSYSCALL is not.
> > > >
> > > > Also, audit_log_name() (and supporting inode and fcaps functions)
should
> > > > have been put back in auditsc.c when soft and hard link logging was
> > > > normalized since it is only used by syscall auditing.
> > > >
> > > > See github issue
https://github.com/linux-audit/audit-kernel/issues/105
> > > >
> > > > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
> > > > ---
> > > > Changelog:
> > > > v2:
> > > > - resolve merge conflicts from rebase on upstreamed ghak103 patch
> > > > - wrap task_struct audit_context in CONFIG_AUDITSYSCALL
> > > >
> > > > include/linux/sched.h | 4 +-
> > > > kernel/audit.c | 157
+++-----------------------------------------------
> > > > kernel/audit.h | 9 ---
> > > > kernel/auditsc.c | 150
+++++++++++++++++++++++++++++++++++++++++++++++
> > > > 4 files changed, 161 insertions(+), 159 deletions(-)
> > >
> > > ...
> > >
> > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > index 3f3f1888cac7..15e41603fd34 100644
> > > > --- a/kernel/audit.c
> > > > +++ b/kernel/audit.c
> > > > @@ -205,7 +205,9 @@ struct audit_net {
> > > > * use simultaneously. */
> > > > struct audit_buffer {
> > > > struct sk_buff *skb; /* formatted skb ready to
send */
> > > > +#ifdef CONFIG_AUDITSYSCALL
> > > > struct audit_context *ctx; /* NULL or associated context
*/
> > > > +#endif
> > > > gfp_t gfp_mask;
> > > > };
> > > >
> > > > @@ -1696,7 +1698,9 @@ static struct audit_buffer
*audit_buffer_alloc(struct audit_context *ctx,
> > > > if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
> > > > goto err;
> > > >
> > > > +#ifdef CONFIG_AUDITSYSCALL
> > > > ab->ctx = ctx;
> > > > +#endif
> > >
> > > I vaguely remember reading/hearing something in the past about
> > > kmem_cache_alloc() not returning a zero'd out buffer in all cases,
can
> > > you say for certain that "ab" in this case is always going to
be
> > > zero'd out? This is an honest question.
> >
> > Ok, then maybe we should be using kmem_cache_zalloc() instead of
> > kmem_cache_alloc() in audit_buffer_alloc()? (as I've done in
> > the last patch of ghak81/first patch of ghak90)
> >
> > If this is too much overhead, then we can initialize ctx = NULL;
>
> We don't need zalloc() since we're setting all the fields, although
> more on this below ...
Ok...
> > > If we can't guarantee that "ab" is zero'd out, we should
manually set
> > > ab->ctx to NULL when !CONFIG_AUDITSYSCALL.
> >
> > But ctx isn't part of struct audit_buffer when !CONFIG_AUDITSYSCALL. It
> > is #ifdef-ed out. What am I missing?
>
> You're not, I am. I saw the obvious bit where you removed it from the
> task_struct, but completely glossed over the bit where you also
> removed it from the audit_buffer struct. My mistake.
>
> Once the audit container ID stuff lands we are going to need to have
> the audit_context pointer in the audit_buffer regardless of the
> CONFIG_AUDITSYSCALL setting, right? Assuming the answer is yes, I
> think I'd just assume leave the pointer in the audit_buffer (setting
> it to NULL when !CONFIG_AUDITSYSCALL) so we don't have to have those
> #ifdef's in the middle of the functions (I generally like to avoid
> those if possible). I think it's still worth making the changes to
> task_struct, as that is the right thing to do and doesn't have the
> same level of impact.
I like to avoid #ifdef compiler directives out when I can too, creating
stubs in the header file to do the job.
Why do we need an audit_context pointer in struct audit_buffer? I'll
take a stab at answering this... I was thinking it wasn't necessary,
but now I think I see what I was missing. I think the only reason is to
connect records to one event through the timestamp and serial number
when syscall is disabled. Up until now it wasn't needed unless full
syscall functionality was present, but once we have an audit container
identifier aux record we will need to join them, at minimum with a local
context for user and netfilter_pkt records.
I also expect us the significance to grow over time as we start to
deal with the event routing problem; one solution would be to track
the audit container ID as a field in the audit_context.
Basically I see the audit_context as the audit "event" data structure
where the audit_buffer is the audit "record" data structure. Their
use doesn't always line up perfectly with those definitions at
present, but I tend to think of the deviations as problems to correct
over time.
So I have to ask, does it make sense to restructure things so that
the
struct audit_buffer has a serial and ctime field so that it isn't needed
in the struct audit_context? I'm not sure if this is possible. I'll
have to go back and look at the code to see if this is the case...
I would say "no" if for no other reason than what I said about about
the audit_context being the "event" data structure.
--
paul moore
www.paul-moore.com
4:24 p.m.
On Thu, Jan 31, 2019 at 10:53 PM Paul Moore <paul(a)paul-moore.com> wrote:
On Tue, Jan 29, 2019 at 9:54 PM Richard Guy Briggs
<rgb(a)redhat.com> wrote:
> On 2019-01-29 18:26, Paul Moore wrote:
> > On Tue, Jan 29, 2019 at 6:18 PM Richard Guy Briggs <rgb(a)redhat.com>
wrote:
> > > On 2019-01-29 18:07, Paul Moore wrote:
> > > > On Mon, Jan 28, 2019 at 1:33 PM Richard Guy Briggs
<rgb(a)redhat.com> wrote:
> > > > > Remove audit_context from struct task_struct and struct
audit_buffer
> > > > > when CONFIG_AUDIT is enabled but CONFIG_AUDITSYSCALL is not.
> > > > >
> > > > > Also, audit_log_name() (and supporting inode and fcaps
functions) should
> > > > > have been put back in auditsc.c when soft and hard link logging
was
> > > > > normalized since it is only used by syscall auditing.
> > > > >
> > > > > See github issue
https://github.com/linux-audit/audit-kernel/issues/105
> > > > >
> > > > > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
> > > > > ---
> > > > > Changelog:
> > > > > v2:
> > > > > - resolve merge conflicts from rebase on upstreamed ghak103
patch
> > > > > - wrap task_struct audit_context in CONFIG_AUDITSYSCALL
> > > > >
> > > > > include/linux/sched.h | 4 +-
> > > > > kernel/audit.c | 157
+++-----------------------------------------------
> > > > > kernel/audit.h | 9 ---
> > > > > kernel/auditsc.c | 150
+++++++++++++++++++++++++++++++++++++++++++++++
> > > > > 4 files changed, 161 insertions(+), 159 deletions(-)
> > > >
> > > > ...
> > > >
> > > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > > index 3f3f1888cac7..15e41603fd34 100644
> > > > > --- a/kernel/audit.c
> > > > > +++ b/kernel/audit.c
> > > > > @@ -205,7 +205,9 @@ struct audit_net {
> > > > > * use simultaneously. */
> > > > > struct audit_buffer {
> > > > > struct sk_buff *skb; /* formatted skb ready
to send */
> > > > > +#ifdef CONFIG_AUDITSYSCALL
> > > > > struct audit_context *ctx; /* NULL or associated
context */
> > > > > +#endif
> > > > > gfp_t gfp_mask;
> > > > > };
> > > > >
> > > > > @@ -1696,7 +1698,9 @@ static struct audit_buffer
*audit_buffer_alloc(struct audit_context *ctx,
> > > > > if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
> > > > > goto err;
> > > > >
> > > > > +#ifdef CONFIG_AUDITSYSCALL
> > > > > ab->ctx = ctx;
> > > > > +#endif
> > > >
> > > > I vaguely remember reading/hearing something in the past about
> > > > kmem_cache_alloc() not returning a zero'd out buffer in all
cases, can
> > > > you say for certain that "ab" in this case is always going
to be
> > > > zero'd out? This is an honest question.
> > >
> > > Ok, then maybe we should be using kmem_cache_zalloc() instead of
> > > kmem_cache_alloc() in audit_buffer_alloc()? (as I've done in
> > > the last patch of ghak81/first patch of ghak90)
> > >
> > > If this is too much overhead, then we can initialize ctx = NULL;
> >
> > We don't need zalloc() since we're setting all the fields, although
> > more on this below ...
>
> Ok...
>
> > > > If we can't guarantee that "ab" is zero'd out, we
should manually set
> > > > ab->ctx to NULL when !CONFIG_AUDITSYSCALL.
> > >
> > > But ctx isn't part of struct audit_buffer when !CONFIG_AUDITSYSCALL.
It
> > > is #ifdef-ed out. What am I missing?
> >
> > You're not, I am. I saw the obvious bit where you removed it from the
> > task_struct, but completely glossed over the bit where you also
> > removed it from the audit_buffer struct. My mistake.
> >
> > Once the audit container ID stuff lands we are going to need to have
> > the audit_context pointer in the audit_buffer regardless of the
> > CONFIG_AUDITSYSCALL setting, right? Assuming the answer is yes, I
> > think I'd just assume leave the pointer in the audit_buffer (setting
> > it to NULL when !CONFIG_AUDITSYSCALL) so we don't have to have those
> > #ifdef's in the middle of the functions (I generally like to avoid
> > those if possible). I think it's still worth making the changes to
> > task_struct, as that is the right thing to do and doesn't have the
> > same level of impact.
>
> I like to avoid #ifdef compiler directives out when I can too, creating
> stubs in the header file to do the job.
>
> Why do we need an audit_context pointer in struct audit_buffer? I'll
> take a stab at answering this... I was thinking it wasn't necessary,
> but now I think I see what I was missing. I think the only reason is to
> connect records to one event through the timestamp and serial number
> when syscall is disabled. Up until now it wasn't needed unless full
> syscall functionality was present, but once we have an audit container
> identifier aux record we will need to join them, at minimum with a local
> context for user and netfilter_pkt records.
I also expect us the significance to grow over time as we start to
deal with the event routing problem; one solution would be to track
the audit container ID as a field in the audit_context.
Basically I see the audit_context as the audit "event" data structure
where the audit_buffer is the audit "record" data structure. Their
use doesn't always line up perfectly with those definitions at
present, but I tend to think of the deviations as problems to correct
over time.
> So I have to ask, does it make sense to restructure things so that the
> struct audit_buffer has a serial and ctime field so that it isn't needed
> in the struct audit_context? I'm not sure if this is possible. I'll
> have to go back and look at the code to see if this is the case...
I would say "no" if for no other reason than what I said about about
the audit_context being the "event" data structure.
Based on your comments in another thread I realize you think I've
queued this for acceptance; I haven't.
I'll be a bit more clear: leave the audit_context pointer in the audit_buffer.
--
paul moore
www.paul-moore.com
4:40 p.m.
On 2019-02-01 17:24, Paul Moore wrote:
On Thu, Jan 31, 2019 at 10:53 PM Paul Moore
<paul(a)paul-moore.com> wrote:
> On Tue, Jan 29, 2019 at 9:54 PM Richard Guy Briggs <rgb(a)redhat.com> wrote:
> > On 2019-01-29 18:26, Paul Moore wrote:
> > > On Tue, Jan 29, 2019 at 6:18 PM Richard Guy Briggs <rgb(a)redhat.com>
wrote:
> > > > On 2019-01-29 18:07, Paul Moore wrote:
> > > > > On Mon, Jan 28, 2019 at 1:33 PM Richard Guy Briggs
<rgb(a)redhat.com> wrote:
> > > > > > Remove audit_context from struct task_struct and struct
audit_buffer
> > > > > > when CONFIG_AUDIT is enabled but CONFIG_AUDITSYSCALL is
not.
> > > > > >
> > > > > > Also, audit_log_name() (and supporting inode and fcaps
functions) should
> > > > > > have been put back in auditsc.c when soft and hard link
logging was
> > > > > > normalized since it is only used by syscall auditing.
> > > > > >
> > > > > > See github issue
https://github.com/linux-audit/audit-kernel/issues/105
> > > > > >
> > > > > > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
> > > > > > ---
> > > > > > Changelog:
> > > > > > v2:
> > > > > > - resolve merge conflicts from rebase on upstreamed ghak103
patch
> > > > > > - wrap task_struct audit_context in CONFIG_AUDITSYSCALL
> > > > > >
> > > > > > include/linux/sched.h | 4 +-
> > > > > > kernel/audit.c | 157
+++-----------------------------------------------
> > > > > > kernel/audit.h | 9 ---
> > > > > > kernel/auditsc.c | 150
+++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > 4 files changed, 161 insertions(+), 159 deletions(-)
> > > > >
> > > > > ...
> > > > >
> > > > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > > > index 3f3f1888cac7..15e41603fd34 100644
> > > > > > --- a/kernel/audit.c
> > > > > > +++ b/kernel/audit.c
> > > > > > @@ -205,7 +205,9 @@ struct audit_net {
> > > > > > * use simultaneously. */
> > > > > > struct audit_buffer {
> > > > > > struct sk_buff *skb; /* formatted skb
ready to send */
> > > > > > +#ifdef CONFIG_AUDITSYSCALL
> > > > > > struct audit_context *ctx; /* NULL or
associated context */
> > > > > > +#endif
> > > > > > gfp_t gfp_mask;
> > > > > > };
> > > > > >
> > > > > > @@ -1696,7 +1698,9 @@ static struct audit_buffer
*audit_buffer_alloc(struct audit_context *ctx,
> > > > > > if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
> > > > > > goto err;
> > > > > >
> > > > > > +#ifdef CONFIG_AUDITSYSCALL
> > > > > > ab->ctx = ctx;
> > > > > > +#endif
> > > > >
> > > > > I vaguely remember reading/hearing something in the past about
> > > > > kmem_cache_alloc() not returning a zero'd out buffer in all
cases, can
> > > > > you say for certain that "ab" in this case is always
going to be
> > > > > zero'd out? This is an honest question.
> > > >
> > > > Ok, then maybe we should be using kmem_cache_zalloc() instead of
> > > > kmem_cache_alloc() in audit_buffer_alloc()? (as I've done in
> > > > the last patch of ghak81/first patch of ghak90)
> > > >
> > > > If this is too much overhead, then we can initialize ctx = NULL;
> > >
> > > We don't need zalloc() since we're setting all the fields,
although
> > > more on this below ...
> >
> > Ok...
> >
> > > > > If we can't guarantee that "ab" is zero'd out,
we should manually set
> > > > > ab->ctx to NULL when !CONFIG_AUDITSYSCALL.
> > > >
> > > > But ctx isn't part of struct audit_buffer when
!CONFIG_AUDITSYSCALL. It
> > > > is #ifdef-ed out. What am I missing?
> > >
> > > You're not, I am. I saw the obvious bit where you removed it from
the
> > > task_struct, but completely glossed over the bit where you also
> > > removed it from the audit_buffer struct. My mistake.
> > >
> > > Once the audit container ID stuff lands we are going to need to have
> > > the audit_context pointer in the audit_buffer regardless of the
> > > CONFIG_AUDITSYSCALL setting, right? Assuming the answer is yes, I
> > > think I'd just assume leave the pointer in the audit_buffer (setting
> > > it to NULL when !CONFIG_AUDITSYSCALL) so we don't have to have those
> > > #ifdef's in the middle of the functions (I generally like to avoid
> > > those if possible). I think it's still worth making the changes to
> > > task_struct, as that is the right thing to do and doesn't have the
> > > same level of impact.
> >
> > I like to avoid #ifdef compiler directives out when I can too, creating
> > stubs in the header file to do the job.
> >
> > Why do we need an audit_context pointer in struct audit_buffer? I'll
> > take a stab at answering this... I was thinking it wasn't necessary,
> > but now I think I see what I was missing. I think the only reason is to
> > connect records to one event through the timestamp and serial number
> > when syscall is disabled. Up until now it wasn't needed unless full
> > syscall functionality was present, but once we have an audit container
> > identifier aux record we will need to join them, at minimum with a local
> > context for user and netfilter_pkt records.
>
> I also expect us the significance to grow over time as we start to
> deal with the event routing problem; one solution would be to track
> the audit container ID as a field in the audit_context.
Agreed.
> Basically I see the audit_context as the audit "event"
data structure
> where the audit_buffer is the audit "record" data structure. Their
> use doesn't always line up perfectly with those definitions at
> present, but I tend to think of the deviations as problems to correct
> over time.
That was starting to become more clear to me after I first read this
paragraph above, having first started to understand it in my previous
reply.
> > So I have to ask, does it make sense to restructure things
so that the
> > struct audit_buffer has a serial and ctime field so that it isn't needed
> > in the struct audit_context? I'm not sure if this is possible. I'll
> > have to go back and look at the code to see if this is the case...
>
> I would say "no" if for no other reason than what I said about about
> the audit_context being the "event" data structure.
Based on your comments in another thread I realize you think I've
queued this for acceptance; I haven't.
No, I've not assumed that at all. It is queued for review, but I've
made no such assumptions it is merged.
I'll be a bit more clear: leave the audit_context pointer in the
audit_buffer.
I had gradually come to realize that distinction between the buffer for
the record and the context for the entire event. I agree the timestamp
and serial number must stay with the event. We could try and make a
furether separation between the event and the context just to hold the
timestamp and serial number but given the possibility of syscall support
coming to the remainer of arches I don't think it is worthwhile.
paul moore
- RGB
--
Richard Guy Briggs <rgb(a)redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
2150
days inactive
2154
days old
linux-audit@lists.linux-audit.osci.io
7 comments
2 participants
participants (2)
-
Paul Moore -
Richard Guy Briggs