[PATCH ghak138 v2 0/4] issues from moving beyond syscalls

[PATCH ghak138 v2 0/4] issues from moving beyond syscalls

[Richard Guy Briggs]

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

[PATCH ghak138 v2 1/4] audit: audit_context pid unused, context enum comment fix

[Richard Guy Briggs]

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 <rgb@redhat.com>
---
 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

Re: [PATCH ghak138 v2 1/4] audit: audit_context pid unused, context enum comment fix

[Paul Moore]

On Thu, Aug 25, 2022 at 3:33 PM Richard Guy Briggs <rgb@redhat.com> 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 <rgb@redhat.com>
> ---
>  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

[PATCH ghak138 v2 2/4] audit: explicitly check audit_context->context enum value

[Richard Guy Briggs]

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 <rgb@redhat.com>
---
 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

Re: [PATCH ghak138 v2 2/4] audit: explicitly check audit_context->context enum value

[Paul Moore]

On Thu, Aug 25, 2022 at 3:33 PM Richard Guy Briggs <rgb@redhat.com> 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 <rgb@redhat.com>
> ---
>  kernel/auditsc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Merged into audit/next, thanks.

-- 
paul-moore.com

[PATCH ghak138 v2 3/4] audit: free audit_proctitle only on task exit

[Richard Guy Briggs]

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 <rgb@redhat.com>
---
 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

Re: [PATCH ghak138 v2 3/4] audit: free audit_proctitle only on task exit

[Paul Moore]

On Thu, Aug 25, 2022 at 3:33 PM Richard Guy Briggs <rgb@redhat.com> 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 <rgb@redhat.com>
> ---
>  kernel/auditsc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Merged into audit/next, thanks.

-- 
paul-moore.com

[PATCH ghak138 v2 4/4] audit: move audit_return_fixup before the filters

[Richard Guy Briggs]

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 <rgb@redhat.com>
---
 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

Re: [PATCH ghak138 v2 4/4] audit: move audit_return_fixup before the filters

[Paul Moore]

On Thu, Aug 25, 2022 at 3:33 PM Richard Guy Briggs <rgb@redhat.com> 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 <rgb@redhat.com>
> ---
>  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