The primary motivation was to solve the mystery of the missing syscall
events filed in ghak138. This is addressed by the audit_return_fixup()
patch and is most likely a stable candidate.
The others were a number of not so critical issues observed in the
process of examining the bisected patch to see what caused it.
changelog v2:
- split into 4 patches
- flesh out proctitle move justification
- add issue reference in return_fixup move patch
- remove explicit Cc:
Richard Guy Briggs (4):
audit: audit_context pid unused, context enum comment fix
audit: explicitly check audit_context->context enum value
audit: free audit_proctitle only on task exit
audit: move audit_return_fixup before the filters
kernel/audit.h | 2 +-
kernel/auditsc.c | 12 ++++++------
2 files changed, 7 insertions(+), 7 deletions(-)
--
2.27.0
Be explicit in checking the struct audit_context "context" member enum
value rather than assuming the order of context enum values.
Fixes: 12c5e81d3fd0 ("audit: prepare audit_context for use in calling contexts beyond syscalls")
Signed-off-by: Richard Guy Briggs <[email protected]>
---
kernel/auditsc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 21e50e6d0fc0..d77c9805c6b1 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2068,7 +2068,7 @@ void __audit_syscall_exit(int success, long return_code)
/* 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)
+ if (context->current_state != AUDIT_STATE_RECORD)
goto out;
audit_return_fixup(context, success, return_code);
--
2.27.0
The success and return_code are needed by the filters. Move
audit_return_fixup() before the filters. This was causing syscall
auditing events to be missed.
Link: https://github.com/linux-audit/audit-kernel/issues/138
Fixes: 12c5e81d3fd0 ("audit: prepare audit_context for use in calling contexts beyond syscalls")
Signed-off-by: Richard Guy Briggs <[email protected]>
---
kernel/auditsc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 280b4720c7a0..9f8c05228d6d 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1940,6 +1940,7 @@ void __audit_uring_exit(int success, long code)
goto out;
}
+ audit_return_fixup(ctx, success, code);
if (ctx->context == AUDIT_CTX_SYSCALL) {
/*
* NOTE: See the note in __audit_uring_entry() about the case
@@ -1981,7 +1982,6 @@ void __audit_uring_exit(int success, long code)
audit_filter_inodes(current, ctx);
if (ctx->current_state != AUDIT_STATE_RECORD)
goto out;
- audit_return_fixup(ctx, success, code);
audit_log_exit();
out:
@@ -2065,13 +2065,13 @@ void __audit_syscall_exit(int success, long return_code)
if (!list_empty(&context->killed_trees))
audit_kill_trees(context);
+ audit_return_fixup(context, success, return_code);
/* 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;
- audit_return_fixup(context, success, return_code);
audit_log_exit();
out:
--
2.27.0
Since audit_proctitle is generated at syscall exit time, its value is
used immediately and cached for the next syscall. Since this is the
case, then only clear it at task exit time. Otherwise, there is no
point in caching the value OR bearing the overhead of regenerating it.
Fixes: 12c5e81d3fd0 ("audit: prepare audit_context for use in calling contexts beyond syscalls")
Signed-off-by: Richard Guy Briggs <[email protected]>
---
kernel/auditsc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index d77c9805c6b1..280b4720c7a0 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1016,7 +1016,6 @@ static void audit_reset_context(struct audit_context *ctx)
WARN_ON(!list_empty(&ctx->killed_trees));
audit_free_module(ctx);
ctx->fds[0] = -1;
- audit_proctitle_free(ctx);
ctx->type = 0; /* reset last for audit_free_*() */
}
@@ -1077,6 +1076,7 @@ static inline void audit_free_context(struct audit_context *context)
{
/* resetting is extra work, but it is likely just noise */
audit_reset_context(context);
+ audit_proctitle_free(context);
free_tree_refs(context);
kfree(context->filterkey);
kfree(context);
--
2.27.0
The pid member of struct audit_context is never used. Remove it.
The audit_reset_context() comment about unconditionally resetting
"ctx->state" should read "ctx->context".
Signed-off-by: Richard Guy Briggs <[email protected]>
---
kernel/audit.h | 2 +-
kernel/auditsc.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/kernel/audit.h b/kernel/audit.h
index 58b66543b4d5..d6eb7b59c791 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -133,7 +133,7 @@ struct audit_context {
struct sockaddr_storage *sockaddr;
size_t sockaddr_len;
/* Save things to print about task_struct */
- pid_t pid, ppid;
+ pid_t ppid;
kuid_t uid, euid, suid, fsuid;
kgid_t gid, egid, sgid, fsgid;
unsigned long personality;
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 9226746dcf0a..21e50e6d0fc0 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -965,7 +965,7 @@ static void audit_reset_context(struct audit_context *ctx)
if (!ctx)
return;
- /* if ctx is non-null, reset the "ctx->state" regardless */
+ /* if ctx is non-null, reset the "ctx->context" regardless */
ctx->context = AUDIT_CTX_UNUSED;
if (ctx->dummy)
return;
@@ -1002,7 +1002,7 @@ static void audit_reset_context(struct audit_context *ctx)
kfree(ctx->sockaddr);
ctx->sockaddr = NULL;
ctx->sockaddr_len = 0;
- ctx->pid = ctx->ppid = 0;
+ 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;
--
2.27.0
On Thu, Aug 25, 2022 at 3:33 PM Richard Guy Briggs <[email protected]> wrote:
>
> The success and return_code are needed by the filters. Move
> audit_return_fixup() before the filters. This was causing syscall
> auditing events to be missed.
>
> Link: https://github.com/linux-audit/audit-kernel/issues/138
> Fixes: 12c5e81d3fd0 ("audit: prepare audit_context for use in calling contexts beyond syscalls")
> Signed-off-by: Richard Guy Briggs <[email protected]>
> ---
> kernel/auditsc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
This looks better. Merged into audit/stable-6.0 and assuming the
automated testing goes well I'll send it to Linus shortly afterward.
--
paul-moore.com
On Thu, Aug 25, 2022 at 3:33 PM Richard Guy Briggs <[email protected]> wrote:
>
> The pid member of struct audit_context is never used. Remove it.
>
> The audit_reset_context() comment about unconditionally resetting
> "ctx->state" should read "ctx->context".
>
> Signed-off-by: Richard Guy Briggs <[email protected]>
> ---
> kernel/audit.h | 2 +-
> kernel/auditsc.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
For future reference, this probably should have been split into two
patches too as there is no connection between the two changes.
However, it's trivial enough I'm just going to merge it now.
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 58b66543b4d5..d6eb7b59c791 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -133,7 +133,7 @@ struct audit_context {
> struct sockaddr_storage *sockaddr;
> size_t sockaddr_len;
> /* Save things to print about task_struct */
> - pid_t pid, ppid;
> + pid_t ppid;
> kuid_t uid, euid, suid, fsuid;
> kgid_t gid, egid, sgid, fsgid;
> unsigned long personality;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 9226746dcf0a..21e50e6d0fc0 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -965,7 +965,7 @@ static void audit_reset_context(struct audit_context *ctx)
> if (!ctx)
> return;
>
> - /* if ctx is non-null, reset the "ctx->state" regardless */
> + /* if ctx is non-null, reset the "ctx->context" regardless */
> ctx->context = AUDIT_CTX_UNUSED;
> if (ctx->dummy)
> return;
> @@ -1002,7 +1002,7 @@ static void audit_reset_context(struct audit_context *ctx)
> kfree(ctx->sockaddr);
> ctx->sockaddr = NULL;
> ctx->sockaddr_len = 0;
> - ctx->pid = ctx->ppid = 0;
> + 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;
> --
> 2.27.0
--
paul-moore.com
On Thu, Aug 25, 2022 at 3:33 PM Richard Guy Briggs <[email protected]> wrote:
>
> Since audit_proctitle is generated at syscall exit time, its value is
> used immediately and cached for the next syscall. Since this is the
> case, then only clear it at task exit time. Otherwise, there is no
> point in caching the value OR bearing the overhead of regenerating it.
>
> Fixes: 12c5e81d3fd0 ("audit: prepare audit_context for use in calling contexts beyond syscalls")
> Signed-off-by: Richard Guy Briggs <[email protected]>
> ---
> kernel/auditsc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Merged into audit/next, thanks.
--
paul-moore.com
On Thu, Aug 25, 2022 at 3:33 PM Richard Guy Briggs <[email protected]> wrote:
>
> Be explicit in checking the struct audit_context "context" member enum
> value rather than assuming the order of context enum values.
>
> Fixes: 12c5e81d3fd0 ("audit: prepare audit_context for use in calling contexts beyond syscalls")
> Signed-off-by: Richard Guy Briggs <[email protected]>
> ---
> kernel/auditsc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Merged into audit/next, thanks.
--
paul-moore.com