[PATCH ghak80 V1] audit: add syscall information to FEATURE

Post by Richard Guy Briggs
Tie syscall information to FEATURE_CHANGE calls since it is a result of
user action.
See: https://github.com/linux-audit/audit-kernel/issues/80
---
kernel/audit.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 8da24ef..23f125b 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1103,10 +1103,9 @@ static void audit_log_feature_change(int which, u32 old_feature, u32 new_feature
{
struct audit_buffer *ab;
- if (audit_enabled == AUDIT_OFF)
+ if (!audit_enabled)

Sooo, this is an unrelated style change, why? Looking at the rest of
kernel/audit.c we seem to use a mix of "(!x)" and "(x == 0/CONST)" so
why are you adding noise to this patch?

Post by Richard Guy Briggs
return;
-
- ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
+ ab = audit_log_start(current->audit_context, GFP_KERNEL, AUDIT_FEATURE_CHANGE);

This is the important part, and the Right Thing To Do.

Post by Richard Guy Briggs
if (!ab)
return;
audit_log_task_info(ab, current);
--
1.8.3.1

--
paul moore
www.paul-moore.com

Steve Grubb

2018-04-17 22:10:30 UTC

Post by Richard Guy Briggs
Tie syscall information to FEATURE_CHANGE calls since it is a result of
user action.
See: https://github.com/linux-audit/audit-kernel/issues/80
---
kernel/audit.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 8da24ef..23f125b 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1103,10 +1103,9 @@ static void audit_log_feature_change(int which,
u32 old_feature, u32 new_feature>
{
struct audit_buffer *ab;
- if (audit_enabled == AUDIT_OFF)
+ if (!audit_enabled)

Sooo, this is an unrelated style change, why? Looking at the rest of
kernel/audit.c we seem to use a mix of "(!x)" and "(x == 0/CONST)" so
why are you adding noise to this patch?

Post by Richard Guy Briggs
return;
-
- ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
+ ab = audit_log_start(current->audit_context, GFP_KERNEL, AUDIT_FEATURE_CHANGE);

This is the important part, and the Right Thing To Do.

This is an unexpected change. I have asked questions on the github issue
tracker but have not gotten a satisfactory answer. Please do not merge this
until there's agreement on this.

Thanks,
-Steve

Post by Richard Guy Briggs
if (!ab)
return;
audit_log_task_info(ab, current);
--
1.8.3.1

Paul Moore

2018-04-18 02:01:46 UTC

Post by Steve Grubb

Post by Richard Guy Briggs
Tie syscall information to FEATURE_CHANGE calls since it is a result of
user action.
See: https://github.com/linux-audit/audit-kernel/issues/80
---
kernel/audit.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 8da24ef..23f125b 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1103,10 +1103,9 @@ static void audit_log_feature_change(int which,
u32 old_feature, u32 new_feature>
{
struct audit_buffer *ab;
- if (audit_enabled == AUDIT_OFF)
+ if (!audit_enabled)

Sooo, this is an unrelated style change, why? Looking at the rest of
kernel/audit.c we seem to use a mix of "(!x)" and "(x == 0/CONST)" so
why are you adding noise to this patch?

Post by Richard Guy Briggs
return;
-
- ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
+ ab = audit_log_start(current->audit_context, GFP_KERNEL,
AUDIT_FEATURE_CHANGE);

This is the important part, and the Right Thing To Do.

This is an unexpected change. I have asked questions on the github issue
tracker but have not gotten a satisfactory answer. Please do not merge this
until there's agreement on this.

It shouldn't be surprising, we've been talking about connecting
records for some time now, in different contexts and both on and off
list. Not only does it helps pave the way for the audit container ID
work, it just makes sense. I've seen your questions in this
particular GitHub issue and I think Richard has answered them
satisfactorily.

Once Richard removes the style change, or provides a good enough
reason for why it should stay in this patch, I plan on merging this
into audit/next.

--
paul moore
www.paul-moore.com

Richard Guy Briggs

2018-04-20 13:46:51 UTC

Sooo, this is an unrelated style change, why? Looking at the rest of
kernel/audit.c we seem to use a mix of "(!x)" and "(x == 0/CONST)" so
why are you adding noise to this patch?

Ok, survey sez 25 instances of audit_enabled used as a boolean vs 7
instances where it could be used as a boolean where the expression is
made harder to read (in my opinion). I thought it was worth changing to
read the same way most of the other instances I've been reviewing are
written. There are only two where the non-boolean distiction with
AUDIT_LOCKED is required.

Post by Richard Guy Briggs
return;
-
- ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
+ ab = audit_log_start(current->audit_context, GFP_KERNEL, AUDIT_FEATURE_CHANGE);

This is the important part, and the Right Thing To Do.

Post by Richard Guy Briggs
if (!ab)
return;
audit_log_task_info(ab, current);

paul moore

- RGB

--
Richard Guy Briggs <***@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

Paul Moore

2018-04-20 15:58:28 UTC

Sooo, this is an unrelated style change, why? Looking at the rest of
kernel/audit.c we seem to use a mix of "(!x)" and "(x == 0/CONST)" so
why are you adding noise to this patch?

Thanks for the explanation.

While I still believe this patch, and connecting records in general,
is the Right Thing To Do, I'm expecting there to be some hate mail on
this issue and I would like to keep the patch as small and as
straight-to-the-point as possible just so there is little confusion
about what is changing.

Please respin this without the style change and I'll merge it as soon
as I see it. Alternatively, give me the "ok" and I'll merge the patch
now and just drop the style change; after all we're talking about one
line in a five line patch ;)

Post by Richard Guy Briggs
return;
-
- ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
+ ab = audit_log_start(current->audit_context, GFP_KERNEL, AUDIT_FEATURE_CHANGE);

This is the important part, and the Right Thing To Do.

Post by Richard Guy Briggs
if (!ab)
return;
audit_log_task_info(ab, current);

--
paul moore
www.paul-moore.com

Richard Guy Briggs

2018-04-20 17:48:12 UTC

Sooo, this is an unrelated style change, why? Looking at the rest of
kernel/audit.c we seem to use a mix of "(!x)" and "(x == 0/CONST)" so
why are you adding noise to this patch?

Thanks for the explanation.
While I still believe this patch, and connecting records in general,
is the Right Thing To Do, I'm expecting there to be some hate mail on
this issue and I would like to keep the patch as small and as
straight-to-the-point as possible just so there is little confusion
about what is changing.
Please respin this without the style change and I'll merge it as soon
as I see it. Alternatively, give me the "ok" and I'll merge the patch
now and just drop the style change; after all we're talking about one
line in a five line patch ;)

Go ahead and drop that style change line to simplify this patch and I'll
submit another patch to clean them all up at the same time (probably the
next time one of those changes).

Post by Richard Guy Briggs
return;
-
- ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
+ ab = audit_log_start(current->audit_context, GFP_KERNEL, AUDIT_FEATURE_CHANGE);

This is the important part, and the Right Thing To Do.

Post by Richard Guy Briggs
if (!ab)
return;
audit_log_task_info(ab, current);

--
paul moore
www.paul-moore.com

- RGB

--
Richard Guy Briggs <***@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

Paul Moore

2018-04-20 19:04:30 UTC