[PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated

[Paul Moore]

Wow, this was a long time ago.

Oh yeah; but it now resurfaced on our side, as we are of course receiving
a lot of requests with respect to making syscall performance great again
:)

Ooof. I'm not sure I can handle making more things "great again" ;)

From memory and a bit of email diving, there are two reasons.
1. The probably was partially solved (by Oleg, IIRC) by making auditctl
-a task,never cause newly spawned tasks to not suck. Yes, it's a
very partial solution. After considerable nagging, I got Fedora to
default to -a task,never.

Hm, right; that's a bit inconvenient, because it takes each and every
vendor having to realize this option, and put it in. Making kernel do the
right thing automatically sounds like a better option to me.

This predates audit falling into my lap, but speaking generally I
think it would be good if the kernel did The Right Thing, so long as
it isn't too painful.

2. This patch, as is, may be a bit problematic. In particular, if one
task changes the audit rules while another task is in the middle of
the syscall, then it's too late to audit that syscall correctly.
This could be seen as a bug or it could be seen as being just fine.

I don't think this should be a problem, given the fact that the whole
timing/ordering is not predictable anyway due to scheduling.
Paul, what do you think?

I'm not overly concerned about the race condition between configuring
the audit filters and syscalls that are currently in-flight; after all
we have that now and "fixing" it would be pretty much impractical
(impossible maybe?). Most serious audit users configure it during
boot and let it run, frequent runtime changes are not common as far as
I can tell.
I just looked quickly at the patch and decided it isn't something I'm
going to be able to carefully review in the time I've got left today,
so it's going to have to wait until tomorrow and Friday ... however,
speaking on general principle I don't have an objection to the ideas
put forth here.
Andy, if you've got any Reviewed-by/Tested-by/NACK/etc. you want to
add, that would be good to have.

... and I just realized that linux-audit isn't on the To/CC line,
adding them now.

Link to the patch is below.

* https://marc.info/?t=152041887600003&r=1&w=2

[Richard Guy Briggs]

Post by Paul Moore

Wow, this was a long time ago.

Oh yeah; but it now resurfaced on our side, as we are of course receiving
a lot of requests with respect to making syscall performance great again
:)

Ooof. I'm not sure I can handle making more things "great again" ;)

From memory and a bit of email diving, there are two reasons.
1. The probably was partially solved (by Oleg, IIRC) by making auditctl
-a task,never cause newly spawned tasks to not suck. Yes, it's a
very partial solution. After considerable nagging, I got Fedora to
default to -a task,never.

Hm, right; that's a bit inconvenient, because it takes each and every
vendor having to realize this option, and put it in. Making kernel do the
right thing automatically sounds like a better option to me.

This predates audit falling into my lap, but speaking generally I
think it would be good if the kernel did The Right Thing, so long as
it isn't too painful.

2. This patch, as is, may be a bit problematic. In particular, if one
task changes the audit rules while another task is in the middle of
the syscall, then it's too late to audit that syscall correctly.
This could be seen as a bug or it could be seen as being just fine.

I don't think this should be a problem, given the fact that the whole
timing/ordering is not predictable anyway due to scheduling.
Paul, what do you think?

I'm not overly concerned about the race condition between configuring
the audit filters and syscalls that are currently in-flight; after all
we have that now and "fixing" it would be pretty much impractical
(impossible maybe?). Most serious audit users configure it during
boot and let it run, frequent runtime changes are not common as far as
I can tell.

I'd agree the race condition here can't easily be fixed and isn't worth
fixing for the reasons already stated (rules don't change often and may
even be locked once in place relatively early, scheduling uncertainties).

Post by Paul Moore

I just looked quickly at the patch and decided it isn't something I'm
going to be able to carefully review in the time I've got left today,
so it's going to have to wait until tomorrow and Friday ... however,
speaking on general principle I don't have an objection to the ideas
put forth here.

The approach seems a bit draconian since it touches all tasks but only
when adding the first rule or deleting the last.

What we lose is the ability to set or clear individual task auditing and
have it stick to speed up non-audited tasks when there are rules
present, though this isn't currently used, in favour of audit_context
presence.

Post by Paul Moore

Andy, if you've got any Reviewed-by/Tested-by/NACK/etc. you want to
add, that would be good to have.

... and I just realized that linux-audit isn't on the To/CC line,
adding them now.

(and Andy's non-NACK missed too...) The mailing list *is* in MAINTAINERS.

Post by Paul Moore
Link to the patch is below.
* https://marc.info/?t=152041887600003&r=1&w=2
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

[Richard Guy Briggs]

Post by Richard Guy Briggs

Post by Paul Moore

Wow, this was a long time ago.

Oh yeah; but it now resurfaced on our side, as we are of course receiving
a lot of requests with respect to making syscall performance great again
:)

Ooof. I'm not sure I can handle making more things "great again" ;)

From memory and a bit of email diving, there are two reasons.
1. The probably was partially solved (by Oleg, IIRC) by making auditctl
-a task,never cause newly spawned tasks to not suck. Yes, it's a
very partial solution. After considerable nagging, I got Fedora to
default to -a task,never.

Hm, right; that's a bit inconvenient, because it takes each and every
vendor having to realize this option, and put it in. Making kernel do the
right thing automatically sounds like a better option to me.

This predates audit falling into my lap, but speaking generally I
think it would be good if the kernel did The Right Thing, so long as
it isn't too painful.

2. This patch, as is, may be a bit problematic. In particular, if one
task changes the audit rules while another task is in the middle of
the syscall, then it's too late to audit that syscall correctly.
This could be seen as a bug or it could be seen as being just fine.

I don't think this should be a problem, given the fact that the whole
timing/ordering is not predictable anyway due to scheduling.
Paul, what do you think?

I'm not overly concerned about the race condition between configuring
the audit filters and syscalls that are currently in-flight; after all
we have that now and "fixing" it would be pretty much impractical
(impossible maybe?). Most serious audit users configure it during
boot and let it run, frequent runtime changes are not common as far as
I can tell.

I'd agree the race condition here can't easily be fixed and isn't worth
fixing for the reasons already stated (rules don't change often and may
even be locked once in place relatively early, scheduling uncertainties).

Post by Paul Moore

I just looked quickly at the patch and decided it isn't something I'm
going to be able to carefully review in the time I've got left today,
so it's going to have to wait until tomorrow and Friday ... however,
speaking on general principle I don't have an objection to the ideas
put forth here.

The approach seems a bit draconian since it touches all tasks but only
when adding the first rule or deleting the last.
What we lose is the ability to set or clear individual task auditing and
have it stick to speed up non-audited tasks when there are rules
present, though this isn't currently used, in favour of audit_context
presence.

Post by Paul Moore

Andy, if you've got any Reviewed-by/Tested-by/NACK/etc. you want to
add, that would be good to have.

... and I just realized that linux-audit isn't on the To/CC line,
adding them now.

(and Andy's non-NACK missed too...) The mailing list *is* in MAINTAINERS.

The mailing list bounces my emails.

They'll get approved.

Post by Richard Guy Briggs

Post by Paul Moore
Link to the patch is below.
* https://marc.info/?t=152041887600003&r=1&w=2
paul moore

- RGB

- 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

[Andy Lutomirski]

Wow, this was a long time ago.

Oh yeah; but it now resurfaced on our side, as we are of course receiving
a lot of requests with respect to making syscall performance great again
:)

Ooof. I'm not sure I can handle making more things "great again" ;)

From memory and a bit of email diving, there are two reasons.
1. The probably was partially solved (by Oleg, IIRC) by making auditctl
-a task,never cause newly spawned tasks to not suck. Yes, it's a
very partial solution. After considerable nagging, I got Fedora to
default to -a task,never.

Hm, right; that's a bit inconvenient, because it takes each and every
vendor having to realize this option, and put it in. Making kernel do the
right thing automatically sounds like a better option to me.

This predates audit falling into my lap, but speaking generally I
think it would be good if the kernel did The Right Thing, so long as
it isn't too painful.

2. This patch, as is, may be a bit problematic. In particular, if one
task changes the audit rules while another task is in the middle of
the syscall, then it's too late to audit that syscall correctly.
This could be seen as a bug or it could be seen as being just fine.

I don't think this should be a problem, given the fact that the whole
timing/ordering is not predictable anyway due to scheduling.
Paul, what do you think?

I'm not overly concerned about the race condition between configuring
the audit filters and syscalls that are currently in-flight; after all
we have that now and "fixing" it would be pretty much impractical
(impossible maybe?). Most serious audit users configure it during
boot and let it run, frequent runtime changes are not common as far as
I can tell.
I just looked quickly at the patch and decided it isn't something I'm
going to be able to carefully review in the time I've got left today,
so it's going to have to wait until tomorrow and Friday ... however,
speaking on general principle I don't have an objection to the ideas
put forth here.
Andy, if you've got any Reviewed-by/Tested-by/NACK/etc. you want to
add, that would be good to have.

It's sort of my patch, and I was reasonably happy with it, so
definitely no NACK from me. The only caveat I have is that I wrote
the original version so long ago that we need to re-audit the code.
In particular, I want to make sure that the following two cases don't
result in warnings, oopses, or other misbehavior:

1. Do a syscall with TIF_AUDIT_SYSCALL clear. Return with
TIF_AUDIT_SYSCALL set and that syscall enabled for auditing.

2.. Do a VFS syscall with TIF_AUDIT_CLEAR and have TIF_AUDIT_SYSCALL
set before we execute any VFS code. The VFS code will call into the
audit code to log the paths it touches (IIRC). Again, this shouldn't
warn or otherwise misbehave.

[Steve Grubb]

On Wed, 7 Mar 2018 18:43:42 -0500

Post by Paul Moore
... and I just realized that linux-audit isn't on the To/CC line,
adding them now.
Link to the patch is below.
* https://marc.info/?t=152041887600003&r=1&w=2

Yes...I wished I was in on the beginning of this discussion. Here's the
problem. We need all tasks auditable unless specifically dismissed as
uninteresting. This would be a task,never rule.

The way we look at it, is if it boots with audit=1, then we know auditd
is expected to run at some point. So, we need all tasks to stay
auditable. If they weren't and auditd enabled auditing, then we'd need
to walk the whole proctable and stab TIF_AUDIT_SYSCALL into every
process in the system. It was decided that this is too ugly.

So, we need them all to be auditable if there is any intent to audit.
It doesn't matter if there are rules loaded or not. All processes have
to stay within reach.

What might be acceptable is to add one more state to audit boot variable
to indicate that auditing is never expected. We currently have:
disabled - which means we'll decide later, enabled, and immutable (no
changes allowed). Then have calls to audit_enable or loading rules
fail on that flag state so that user space can log that there is a
conflict (boot vs daemon) that has to be resolved. As long as we can
fail in a discoverable way, I think it would be OK to do something like
this. Also, I don't think we want that to be the default state at the
moment because the current default is keep all processes auditable.

-Steve

[Andy Lutomirski]

Post by Steve Grubb
On Wed, 7 Mar 2018 18:43:42 -0500

Post by Paul Moore
... and I just realized that linux-audit isn't on the To/CC line,
adding them now.
Link to the patch is below.
* https://marc.info/?t=152041887600003&r=1&w=2

Yes...I wished I was in on the beginning of this discussion. Here's the
problem. We need all tasks auditable unless specifically dismissed as
uninteresting. This would be a task,never rule.
The way we look at it, is if it boots with audit=1, then we know auditd
is expected to run at some point. So, we need all tasks to stay
auditable. If they weren't and auditd enabled auditing, then we'd need
to walk the whole proctable and stab TIF_AUDIT_SYSCALL into every
process in the system. It was decided that this is too ugly.

When was that decided? That's what this patch does.

[Jiri Kosina]

Post by Andy Lutomirski

Post by Steve Grubb
Yes...I wished I was in on the beginning of this discussion. Here's the
problem. We need all tasks auditable unless specifically dismissed as
uninteresting. This would be a task,never rule.
The way we look at it, is if it boots with audit=1, then we know auditd
is expected to run at some point. So, we need all tasks to stay
auditable. If they weren't and auditd enabled auditing, then we'd need
to walk the whole proctable and stab TIF_AUDIT_SYSCALL into every
process in the system. It was decided that this is too ugly.

When was that decided? That's what this patch does.

I'd like to see some more justification as well.

Namely, if I compare "setting TIF_AUDIT_SYSCALL for every process on a
need-to-be-so basis" to "we always go through the slow path and
pessimistically assume that audit is enabled and has reasonable ruleset
loaded", I have my own (different) opinion of what is too ugly.

Thanks,

[Andy Lutomirski]

Post by Jiri Kosina

Post by Andy Lutomirski

Post by Steve Grubb
Yes...I wished I was in on the beginning of this discussion. Here's the
problem. We need all tasks auditable unless specifically dismissed as
uninteresting. This would be a task,never rule.
The way we look at it, is if it boots with audit=1, then we know auditd
is expected to run at some point. So, we need all tasks to stay
auditable. If they weren't and auditd enabled auditing, then we'd need
to walk the whole proctable and stab TIF_AUDIT_SYSCALL into every
process in the system. It was decided that this is too ugly.

When was that decided? That's what this patch does.

I'd like to see some more justification as well.
Namely, if I compare "setting TIF_AUDIT_SYSCALL for every process on a
need-to-be-so basis" to "we always go through the slow path and
pessimistically assume that audit is enabled and has reasonable ruleset
loaded", I have my own (different) opinion of what is too ugly.

Me too.

That being said, on re-review of my old code, I think that
audit_dec_n_rules() may be the wrong approach. It may be better to
leave TIF_AUDIT_SYSCALL set but to make the audit code itself clear
the flag the next time through. That way we don't end up with a
partially filled in syscall audit record that never gets consumed if
we clear TIF_AUDIT_SYSCALL in the middle of a syscall.

[Steve Grubb]

Post by Andy Lutomirski

Post by Jiri Kosina

Post by Andy Lutomirski

Post by Steve Grubb
Yes...I wished I was in on the beginning of this discussion. Here's the
problem. We need all tasks auditable unless specifically dismissed as
uninteresting. This would be a task,never rule.
The way we look at it, is if it boots with audit=1, then we know auditd
is expected to run at some point. So, we need all tasks to stay
auditable. If they weren't and auditd enabled auditing, then we'd need
to walk the whole proctable and stab TIF_AUDIT_SYSCALL into every
process in the system. It was decided that this is too ugly.

When was that decided? That's what this patch does.

I'd like to see some more justification as well.
Namely, if I compare "setting TIF_AUDIT_SYSCALL for every process on a
need-to-be-so basis" to "we always go through the slow path and
pessimistically assume that audit is enabled and has reasonable ruleset
loaded", I have my own (different) opinion of what is too ugly.

Me too.
That being said, on re-review of my old code, I think that
audit_dec_n_rules() may be the wrong approach. It may be better to
leave TIF_AUDIT_SYSCALL set but to make the audit code itself clear
the flag the next time through. That way we don't end up with a
partially filled in syscall audit record that never gets consumed if
we clear TIF_AUDIT_SYSCALL in the middle of a syscall.

So what happened when auditd is being restarted due to system update? Its
possible to have no auditd, no rules, and audit_enabled == 0. But its getting
ready to start a new auditd, enable audit, and load rules. In this case, we
expect tasks already dismissed by task filter to stay dismissed because the
task filter only runs at fork time.

For example, suppose httpd is not desired to be audited. The admin sets up a
task rule that sets it to never for httpd. The rule triggers at fork and
marks it inauditable. Would this patch cause httpd to suddenly become
auditable again during an auditd restart?

Rather than play run time games which is ultimately racey and prone to
missing something, what about the approach of controlling this from a boot
variable and letting user space see an error should auditing try to do its
normal thing when its not wanted? This way auditd can exit and there aren't
any races.

-Steve

[Steve Grubb]

Post by Jiri Kosina

Post by Andy Lutomirski

Post by Steve Grubb
Yes...I wished I was in on the beginning of this discussion. Here's the
problem. We need all tasks auditable unless specifically dismissed as
uninteresting. This would be a task,never rule.
The way we look at it, is if it boots with audit=1, then we know auditd
is expected to run at some point. So, we need all tasks to stay
auditable. If they weren't and auditd enabled auditing, then we'd need
to walk the whole proctable and stab TIF_AUDIT_SYSCALL into every
process in the system. It was decided that this is too ugly.

When was that decided? That's what this patch does.

I'd like to see some more justification as well.

There was some discussion about removing the flag here:
https://www.redhat.com/archives/linux-audit/2007-October/msg00053.html

-Steve

Post by Jiri Kosina
Namely, if I compare "setting TIF_AUDIT_SYSCALL for every process on a
need-to-be-so basis" to "we always go through the slow path and
pessimistically assume that audit is enabled and has reasonable ruleset
loaded", I have my own (different) opinion of what is too ugly.