issues from moving beyond syscalls

PATCH Applied Richard Guy Briggs 2 participants 9 messages Aug 25, 2022 – Aug 26, 2022 Audit

Share & tools

Richard Guy Briggs

author

2022-08-25 19:33 UTC #0 lore raw

v2 cover issues from moving beyond syscalls

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(-)

Signature · 6 lines

2.27.0

--
Linux-audit mailing list
[email protected]
https://listman.redhat.com/mailman/listinfo/linux-audit

Richard Guy Briggs

author

2022-08-25 19:33 UTC #1 lore raw

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

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(-)

kernel/audit.h +1 -1

index 58b66543b4d5..d6eb7b59c791 100644
@@ -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;

kernel/auditsc.c +2 -2

index 9226746dcf0a..21e50e6d0fc0 100644
@@ -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;

Signature · 6 lines

2.27.0

--
Linux-audit mailing list
[email protected]
https://listman.redhat.com/mailman/listinfo/linux-audit

Paul Moore

maintainer

2022-08-26 21:16 UTC #2 lore raw

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

Quoted 11 lines · 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 <[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.

Quoted 37 lines

kernel/audit.h +1 -1

index 58b66543b4d5..d6eb7b59c791 100644
@@ -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;

kernel/auditsc.c +2 -3

index 9226746dcf0a..21e50e6d0fc0 100644
@@ -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

Signature · 1 line

paul-moore.com

Richard Guy Briggs

author

2022-08-25 19:33 UTC #3 lore raw

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

In reply to 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 <[email protected]>

---
 kernel/auditsc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/auditsc.c +1 -1

index 21e50e6d0fc0..d77c9805c6b1 100644
@@ -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);

Signature · 6 lines

2.27.0

--
Linux-audit mailing list
[email protected]
https://listman.redhat.com/mailman/listinfo/linux-audit

Paul Moore

maintainer Applied

2022-08-26 21:18 UTC #4 lore raw

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

Quoted 9 lines · 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 <[email protected]>
---
 kernel/auditsc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Merged into audit/next, thanks.

Signature · 1 line

paul-moore.com

Richard Guy Briggs

author

2022-08-25 19:33 UTC #5 lore raw

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

In reply to 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 <[email protected]>

---
 kernel/auditsc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/auditsc.c +1 -1

index d77c9805c6b1..280b4720c7a0 100644
@@ -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);

Signature · 6 lines

2.27.0

--
Linux-audit mailing list
[email protected]
https://listman.redhat.com/mailman/listinfo/linux-audit

Paul Moore

maintainer Applied

2022-08-26 21:20 UTC #6 lore raw

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

Quoted 11 lines · 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 <[email protected]>
---
 kernel/auditsc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Merged into audit/next, thanks.

Signature · 6 lines

paul-moore.com

--
Linux-audit mailing list
[email protected]
https://listman.redhat.com/mailman/listinfo/linux-audit

Richard Guy Briggs

author

2022-08-25 19:33 UTC #7 lore raw

PATCH v2 4/4 audit: move audit_return_fixup before the filters

In reply to 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 <[email protected]>

---
 kernel/auditsc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

kernel/auditsc.c +2 -2

index 280b4720c7a0..9f8c05228d6d 100644
@@ -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:

Signature · 6 lines

2.27.0

--
Linux-audit mailing list
[email protected]
https://listman.redhat.com/mailman/listinfo/linux-audit

Paul Moore

maintainer

2022-08-25 21:36 UTC #8 lore raw

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

Quoted 11 lines · 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 <[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.

Signature · 1 line

paul-moore.com