* [PATCH] Fixed reason field in audit signal logging
@ 2013-11-07 13:39 Paul Davies C
2013-11-07 14:43 ` Eric Paris
0 siblings, 1 reply; 12+ messages in thread
From: Paul Davies C @ 2013-11-07 13:39 UTC (permalink / raw)
To: linux-audit, viro, eparis
The audit system logs the signals that leads to abnormal end of a process.
However , as of now , it always states the reason for failure of a process as
"memory violation" regardless of the signal delivered. This is due to the
audit_core_dumps() function pass the reason for failure blindly to the
audit_log_abend() as "memory violation".
This patch changes the audit_core_dumps() function as to pass on the right
reason to the audit_log_abend based on the signal received.
Signed-off-by:Paul Davies C
---
kernel/auditsc.c | 31 ++++++++++++++++++++++++++++++-
1 file changed, 30 insertions(+), 1 deletion(-)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 9845cb3..3cafd13 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2395,7 +2395,36 @@ void audit_core_dumps(long signr)
ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND);
if (unlikely(!ab))
return;
- audit_log_abend(ab, "memory violation", signr);
+
+ /*Identify the reason for failure based on signal delivered.*/
+ switch (signr) {
+ case SIGABRT:
+ audit_log_abend(ab, "received abort", signr);
+ break;
+ case SIGBUS:
+ audit_log_abend(ab, "invalid pointer dereference", signr);
+ break;
+ case SIGFPE:
+ audit_log_abend(ab, "invalid floating point instruction", signr);
+ break;
+ case SIGILL:
+ audit_log_abend(ab, "illegal instruction", signr);
+ break;
+ case SIGSEGV:
+ audit_log_abend(ab, "memory violation", signr);
+ break;
+ case SIGTRAP:
+ audit_log_abend(ab, "bad instruction / debugger generated signal", signr);
+ break;
+ case SIGXCPU:
+ audit_log_abend(ab, "cpu time violation", signr);
+ break;
+ case SIGXFSZ:
+ audit_log_abend(ab, "file size violation", signr);
+ break;
+ default:
+ audit_log_abend(ab, "not defined", signr);
+ }
audit_log_end(ab);
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] Fixed reason field in audit signal logging
2013-11-07 13:39 [PATCH] Fixed reason field in audit signal logging Paul Davies C
@ 2013-11-07 14:43 ` Eric Paris
2013-11-07 14:52 ` LC Bruzenak
2013-11-07 15:05 ` Steve Grubb
0 siblings, 2 replies; 12+ messages in thread
From: Eric Paris @ 2013-11-07 14:43 UTC (permalink / raw)
To: Paul Davies C; +Cc: linux-audit, viro
On Thu, 2013-11-07 at 19:09 +0530, Paul Davies C wrote:
> The audit system logs the signals that leads to abnormal end of a process.
> However , as of now , it always states the reason for failure of a process as
> "memory violation" regardless of the signal delivered. This is due to the
> audit_core_dumps() function pass the reason for failure blindly to the
> audit_log_abend() as "memory violation".
>
> This patch changes the audit_core_dumps() function as to pass on the right
> reason to the audit_log_abend based on the signal received.
>
> Signed-off-by:Paul Davies C
Acked-by: Eric Paris <eparis@redhat.com>
But we really should wait for an Ack and thoughts from steve grubb....
> ---
> kernel/auditsc.c | 31 ++++++++++++++++++++++++++++++-
> 1 file changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 9845cb3..3cafd13 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2395,7 +2395,36 @@ void audit_core_dumps(long signr)
> ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND);
> if (unlikely(!ab))
> return;
> - audit_log_abend(ab, "memory violation", signr);
> +
> + /*Identify the reason for failure based on signal delivered.*/
> + switch (signr) {
> + case SIGABRT:
> + audit_log_abend(ab, "received abort", signr);
> + break;
> + case SIGBUS:
> + audit_log_abend(ab, "invalid pointer dereference", signr);
> + break;
> + case SIGFPE:
> + audit_log_abend(ab, "invalid floating point instruction", signr);
> + break;
> + case SIGILL:
> + audit_log_abend(ab, "illegal instruction", signr);
> + break;
> + case SIGSEGV:
> + audit_log_abend(ab, "memory violation", signr);
> + break;
> + case SIGTRAP:
> + audit_log_abend(ab, "bad instruction / debugger generated signal", signr);
> + break;
> + case SIGXCPU:
> + audit_log_abend(ab, "cpu time violation", signr);
> + break;
> + case SIGXFSZ:
> + audit_log_abend(ab, "file size violation", signr);
> + break;
> + default:
> + audit_log_abend(ab, "not defined", signr);
> + }
> audit_log_end(ab);
> }
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fixed reason field in audit signal logging
2013-11-07 14:43 ` Eric Paris
@ 2013-11-07 14:52 ` LC Bruzenak
2013-11-07 15:05 ` Steve Grubb
1 sibling, 0 replies; 12+ messages in thread
From: LC Bruzenak @ 2013-11-07 14:52 UTC (permalink / raw)
To: linux-audit
On 11/07/2013 08:43 AM, Eric Paris wrote:
> On Thu, 2013-11-07 at 19:09 +0530, Paul Davies C wrote:
>> The audit system logs the signals that leads to abnormal end of a process.
>> However , as of now , it always states the reason for failure of a process as
>> "memory violation" regardless of the signal delivered. This is due to the
>> audit_core_dumps() function pass the reason for failure blindly to the
>> audit_log_abend() as "memory violation".
>>
>> This patch changes the audit_core_dumps() function as to pass on the right
>> reason to the audit_log_abend based on the signal received.
>>
>> Signed-off-by:Paul Davies C
> Acked-by: Eric Paris <eparis@redhat.com>
>
> But we really should wait for an Ack and thoughts from steve grubb....
>
FWIW I really like this fix. It will be valuable to me (once I get it in
my RHEL-6.2 systems); thanks!
LCB
--
LC (Lenny) Bruzenak
lenny@magitekltd.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fixed reason field in audit signal logging
2013-11-07 14:43 ` Eric Paris
2013-11-07 14:52 ` LC Bruzenak
@ 2013-11-07 15:05 ` Steve Grubb
2013-11-07 15:13 ` LC Bruzenak
2013-11-07 15:42 ` Eric Paris
1 sibling, 2 replies; 12+ messages in thread
From: Steve Grubb @ 2013-11-07 15:05 UTC (permalink / raw)
To: Eric Paris; +Cc: linux-audit, viro
On Thursday, November 07, 2013 09:43:24 AM Eric Paris wrote:
> On Thu, 2013-11-07 at 19:09 +0530, Paul Davies C wrote:
> > The audit system logs the signals that leads to abnormal end of a process.
> > However , as of now , it always states the reason for failure of a process
> > as "memory violation" regardless of the signal delivered. This is due to
> > the audit_core_dumps() function pass the reason for failure blindly to
> > the audit_log_abend() as "memory violation".
> >
> > This patch changes the audit_core_dumps() function as to pass on the right
> > reason to the audit_log_abend based on the signal received.
> >
> > Signed-off-by:Paul Davies C
>
> Acked-by: Eric Paris <eparis@redhat.com>
>
> But we really should wait for an Ack and thoughts from steve grubb....
I am confused. This is the abnormal end event I have:
type=ANOM_ABEND msg=audit(1303339663.307:142): auid=4325 uid=0 gid=0 ses=1
subj=unconfined_u:unconfined_r:unconfined_t:s0 pid=3775 comm="aureport" sig=11
Why / when did we start adding text explanations? We should not do that. We
didn't have it before and it should not have been added. The signal number is
enough to identify the problem.
If we did need a reason= field, all these strings with spaces will get
separated on parsing. They should be like "memory-violation" or "recieved-
abort". And would it be better to hide this in the audit_log_abend function? I
honestly don't understand why this was added.
-Steve
> > ---
> >
> > kernel/auditsc.c | 31 ++++++++++++++++++++++++++++++-
> > 1 file changed, 30 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 9845cb3..3cafd13 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -2395,7 +2395,36 @@ void audit_core_dumps(long signr)
> >
> > ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND);
> > if (unlikely(!ab))
> >
> > return;
> >
> > - audit_log_abend(ab, "memory violation", signr);
> > +
> > + /*Identify the reason for failure based on signal delivered.*/
> > + switch (signr) {
> > + case SIGABRT:
> > + audit_log_abend(ab, "received abort", signr);
> > + break;
> > + case SIGBUS:
> > + audit_log_abend(ab, "invalid pointer dereference", signr);
> > + break;
> > + case SIGFPE:
> > + audit_log_abend(ab, "invalid floating point instruction",
signr);
> > + break;
> > + case SIGILL:
> > + audit_log_abend(ab, "illegal instruction", signr);
> > + break;
> > + case SIGSEGV:
> > + audit_log_abend(ab, "memory violation", signr);
> > + break;
> > + case SIGTRAP:
> > + audit_log_abend(ab, "bad instruction / debugger generated
signal",
> > signr); + break;
> > + case SIGXCPU:
> > + audit_log_abend(ab, "cpu time violation", signr);
> > + break;
> > + case SIGXFSZ:
> > + audit_log_abend(ab, "file size violation", signr);
> > + break;
> > + default:
> > + audit_log_abend(ab, "not defined", signr);
> > + }
> >
> > audit_log_end(ab);
> >
> > }
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fixed reason field in audit signal logging
2013-11-07 15:05 ` Steve Grubb
@ 2013-11-07 15:13 ` LC Bruzenak
2013-11-07 15:42 ` Eric Paris
1 sibling, 0 replies; 12+ messages in thread
From: LC Bruzenak @ 2013-11-07 15:13 UTC (permalink / raw)
To: linux-audit
On 11/07/2013 09:05 AM, Steve Grubb wrote:
>
> I am confused. This is the abnormal end event I have:
>
> type=ANOM_ABEND msg=audit(1303339663.307:142): auid=4325 uid=0 gid=0 ses=1
> subj=unconfined_u:unconfined_r:unconfined_t:s0 pid=3775 comm="aureport" sig=11
>
> Why / when did we start adding text explanations? We should not do that. We
> didn't have it before and it should not have been added. The signal number is
> enough to identify the problem.
>
> If we did need a reason= field, all these strings with spaces will get
> separated on parsing. They should be like "memory-violation" or "recieved-
> abort". And would it be better to hide this in the audit_log_abend function? I
> honestly don't understand why this was added.
>
> -Steve
Whoops; looks like I jumped the gun. I also have the same results:
node=test1 type=ANOM_ABEND msg=audit(1383674813.174:5025253):
auid=4294967295 uid=0 gid=0 ses=4294967295
subj=system_u:system_r:xserver_t:s0-s15:c0.c1023 pid=5537 comm="X" sig=6
It looked like it would add value at first read.
LCB
--
LC (Lenny) Bruzenak
lenny@magitekltd.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fixed reason field in audit signal logging
2013-11-07 15:05 ` Steve Grubb
2013-11-07 15:13 ` LC Bruzenak
@ 2013-11-07 15:42 ` Eric Paris
2013-11-07 15:51 ` Paul Davies C
2013-11-07 16:11 ` Steve Grubb
1 sibling, 2 replies; 12+ messages in thread
From: Eric Paris @ 2013-11-07 15:42 UTC (permalink / raw)
To: Steve Grubb; +Cc: linux-audit, viro
On Thu, 2013-11-07 at 10:05 -0500, Steve Grubb wrote:
> On Thursday, November 07, 2013 09:43:24 AM Eric Paris wrote:
> > On Thu, 2013-11-07 at 19:09 +0530, Paul Davies C wrote:
> > > The audit system logs the signals that leads to abnormal end of a process.
> > > However , as of now , it always states the reason for failure of a process
> > > as "memory violation" regardless of the signal delivered. This is due to
> > > the audit_core_dumps() function pass the reason for failure blindly to
> > > the audit_log_abend() as "memory violation".
> > >
> > > This patch changes the audit_core_dumps() function as to pass on the right
> > > reason to the audit_log_abend based on the signal received.
> > >
> > > Signed-off-by:Paul Davies C
> >
> > Acked-by: Eric Paris <eparis@redhat.com>
> >
> > But we really should wait for an Ack and thoughts from steve grubb....
>
> I am confused. This is the abnormal end event I have:
>
> type=ANOM_ABEND msg=audit(1303339663.307:142): auid=4325 uid=0 gid=0 ses=1
> subj=unconfined_u:unconfined_r:unconfined_t:s0 pid=3775 comm="aureport" sig=11
>
> Why / when did we start adding text explanations? We should not do that. We
> didn't have it before and it should not have been added. The signal number is
> enough to identify the problem.
We started adding a reason when seccomp started sending ANOM_ABEND
events as well. It doesn't do so with a signal. Agreed, the " " is/was
a bad idea...
>
> If we did need a reason= field, all these strings with spaces will get
> separated on parsing. They should be like "memory-violation" or "recieved-
> abort". And would it be better to hide this in the audit_log_abend function? I
> honestly don't understand why this was added.
>
> -Steve
>
>
> > > ---
> > >
> > > kernel/auditsc.c | 31 ++++++++++++++++++++++++++++++-
> > > 1 file changed, 30 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > index 9845cb3..3cafd13 100644
> > > --- a/kernel/auditsc.c
> > > +++ b/kernel/auditsc.c
> > > @@ -2395,7 +2395,36 @@ void audit_core_dumps(long signr)
> > >
> > > ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND);
> > > if (unlikely(!ab))
> > >
> > > return;
> > >
> > > - audit_log_abend(ab, "memory violation", signr);
> > > +
> > > + /*Identify the reason for failure based on signal delivered.*/
> > > + switch (signr) {
> > > + case SIGABRT:
> > > + audit_log_abend(ab, "received abort", signr);
> > > + break;
> > > + case SIGBUS:
> > > + audit_log_abend(ab, "invalid pointer dereference", signr);
> > > + break;
> > > + case SIGFPE:
> > > + audit_log_abend(ab, "invalid floating point instruction",
> signr);
> > > + break;
> > > + case SIGILL:
> > > + audit_log_abend(ab, "illegal instruction", signr);
> > > + break;
> > > + case SIGSEGV:
> > > + audit_log_abend(ab, "memory violation", signr);
> > > + break;
> > > + case SIGTRAP:
> > > + audit_log_abend(ab, "bad instruction / debugger generated
> signal",
> > > signr); + break;
> > > + case SIGXCPU:
> > > + audit_log_abend(ab, "cpu time violation", signr);
> > > + break;
> > > + case SIGXFSZ:
> > > + audit_log_abend(ab, "file size violation", signr);
> > > + break;
> > > + default:
> > > + audit_log_abend(ab, "not defined", signr);
> > > + }
> > >
> > > audit_log_end(ab);
> > >
> > > }
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fixed reason field in audit signal logging
2013-11-07 15:42 ` Eric Paris
@ 2013-11-07 15:51 ` Paul Davies C
2013-11-07 15:53 ` Eric Paris
2013-11-07 16:11 ` Steve Grubb
1 sibling, 1 reply; 12+ messages in thread
From: Paul Davies C @ 2013-11-07 15:51 UTC (permalink / raw)
To: Eric Paris; +Cc: viro, linux-audit
[-- Attachment #1.1: Type: text/plain, Size: 4115 bytes --]
So rather than logging the "reason=memory violation" when process ends
abnormally due to any signal delivery , it will be be better if we leave
"reason=undefined" .
Your thoughts?
On Thu, Nov 7, 2013 at 9:12 PM, Eric Paris <eparis@redhat.com> wrote:
> On Thu, 2013-11-07 at 10:05 -0500, Steve Grubb wrote:
> > On Thursday, November 07, 2013 09:43:24 AM Eric Paris wrote:
> > > On Thu, 2013-11-07 at 19:09 +0530, Paul Davies C wrote:
> > > > The audit system logs the signals that leads to abnormal end of a
> process.
> > > > However , as of now , it always states the reason for failure of a
> process
> > > > as "memory violation" regardless of the signal delivered. This is
> due to
> > > > the audit_core_dumps() function pass the reason for failure blindly
> to
> > > > the audit_log_abend() as "memory violation".
> > > >
> > > > This patch changes the audit_core_dumps() function as to pass on the
> right
> > > > reason to the audit_log_abend based on the signal received.
> > > >
> > > > Signed-off-by:Paul Davies C
> > >
> > > Acked-by: Eric Paris <eparis@redhat.com>
> > >
> > > But we really should wait for an Ack and thoughts from steve grubb....
> >
> > I am confused. This is the abnormal end event I have:
> >
> > type=ANOM_ABEND msg=audit(1303339663.307:142): auid=4325 uid=0 gid=0
> ses=1
> > subj=unconfined_u:unconfined_r:unconfined_t:s0 pid=3775 comm="aureport"
> sig=11
> >
> > Why / when did we start adding text explanations? We should not do that.
> We
> > didn't have it before and it should not have been added. The signal
> number is
> > enough to identify the problem.
>
> We started adding a reason when seccomp started sending ANOM_ABEND
> events as well. It doesn't do so with a signal. Agreed, the " " is/was
> a bad idea...
>
> >
> > If we did need a reason= field, all these strings with spaces will get
> > separated on parsing. They should be like "memory-violation" or
> "recieved-
> > abort". And would it be better to hide this in the audit_log_abend
> function? I
> > honestly don't understand why this was added.
> >
> > -Steve
> >
> >
> > > > ---
> > > >
> > > > kernel/auditsc.c | 31 ++++++++++++++++++++++++++++++-
> > > > 1 file changed, 30 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > index 9845cb3..3cafd13 100644
> > > > --- a/kernel/auditsc.c
> > > > +++ b/kernel/auditsc.c
> > > > @@ -2395,7 +2395,36 @@ void audit_core_dumps(long signr)
> > > >
> > > > ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND);
> > > > if (unlikely(!ab))
> > > >
> > > > return;
> > > >
> > > > - audit_log_abend(ab, "memory violation", signr);
> > > > +
> > > > + /*Identify the reason for failure based on signal delivered.*/
> > > > + switch (signr) {
> > > > + case SIGABRT:
> > > > + audit_log_abend(ab, "received abort", signr);
> > > > + break;
> > > > + case SIGBUS:
> > > > + audit_log_abend(ab, "invalid pointer dereference",
> signr);
> > > > + break;
> > > > + case SIGFPE:
> > > > + audit_log_abend(ab, "invalid floating point
> instruction",
> > signr);
> > > > + break;
> > > > + case SIGILL:
> > > > + audit_log_abend(ab, "illegal instruction", signr);
> > > > + break;
> > > > + case SIGSEGV:
> > > > + audit_log_abend(ab, "memory violation", signr);
> > > > + break;
> > > > + case SIGTRAP:
> > > > + audit_log_abend(ab, "bad instruction / debugger
> generated
> > signal",
> > > > signr); + break;
> > > > + case SIGXCPU:
> > > > + audit_log_abend(ab, "cpu time violation", signr);
> > > > + break;
> > > > + case SIGXFSZ:
> > > > + audit_log_abend(ab, "file size violation", signr);
> > > > + break;
> > > > + default:
> > > > + audit_log_abend(ab, "not defined", signr);
> > > > + }
> > > >
> > > > audit_log_end(ab);
> > > >
> > > > }
> >
>
>
>
--
*Regards,*
*Paul Davies C*
vivafoss.blogspot.com
[-- Attachment #1.2: Type: text/html, Size: 5925 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fixed reason field in audit signal logging
2013-11-07 15:51 ` Paul Davies C
@ 2013-11-07 15:53 ` Eric Paris
2013-11-07 16:00 ` Paul Davies C
0 siblings, 1 reply; 12+ messages in thread
From: Eric Paris @ 2013-11-07 15:53 UTC (permalink / raw)
To: Paul Davies C; +Cc: viro, linux-audit
On Thu, 2013-11-07 at 21:21 +0530, Paul Davies C wrote:
> So rather than logging the "reason=memory violation" when process ends
> abnormally due to any signal delivery , it will be be better if we
> leave "reason=undefined" .
reason=memory_violation or invalid_pointer etc
although maybe it should be just 'signal'... and you can get the signal
number from the record....
> Your thoughts?
>
>
> On Thu, Nov 7, 2013 at 9:12 PM, Eric Paris <eparis@redhat.com> wrote:
> On Thu, 2013-11-07 at 10:05 -0500, Steve Grubb wrote:
> > On Thursday, November 07, 2013 09:43:24 AM Eric Paris wrote:
> > > On Thu, 2013-11-07 at 19:09 +0530, Paul Davies C wrote:
> > > > The audit system logs the signals that leads to abnormal
> end of a process.
> > > > However , as of now , it always states the reason for
> failure of a process
> > > > as "memory violation" regardless of the signal
> delivered. This is due to
> > > > the audit_core_dumps() function pass the reason for
> failure blindly to
> > > > the audit_log_abend() as "memory violation".
> > > >
> > > > This patch changes the audit_core_dumps() function as to
> pass on the right
> > > > reason to the audit_log_abend based on the signal
> received.
> > > >
> > > > Signed-off-by:Paul Davies C
> > >
> > > Acked-by: Eric Paris <eparis@redhat.com>
> > >
> > > But we really should wait for an Ack and thoughts from
> steve grubb....
> >
> > I am confused. This is the abnormal end event I have:
> >
> > type=ANOM_ABEND msg=audit(1303339663.307:142): auid=4325
> uid=0 gid=0 ses=1
> > subj=unconfined_u:unconfined_r:unconfined_t:s0 pid=3775
> comm="aureport" sig=11
> >
> > Why / when did we start adding text explanations? We should
> not do that. We
> > didn't have it before and it should not have been added. The
> signal number is
> > enough to identify the problem.
>
>
> We started adding a reason when seccomp started sending
> ANOM_ABEND
> events as well. It doesn't do so with a signal. Agreed, the
> " " is/was
> a bad idea...
>
> >
> > If we did need a reason= field, all these strings with
> spaces will get
> > separated on parsing. They should be like "memory-violation"
> or "recieved-
> > abort". And would it be better to hide this in the
> audit_log_abend function? I
> > honestly don't understand why this was added.
> >
> > -Steve
> >
> >
> > > > ---
> > > >
> > > > kernel/auditsc.c | 31 ++++++++++++++++++++++++++++++-
> > > > 1 file changed, 30 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > index 9845cb3..3cafd13 100644
> > > > --- a/kernel/auditsc.c
> > > > +++ b/kernel/auditsc.c
> > > > @@ -2395,7 +2395,36 @@ void audit_core_dumps(long signr)
> > > >
> > > > ab = audit_log_start(NULL, GFP_KERNEL,
> AUDIT_ANOM_ABEND);
> > > > if (unlikely(!ab))
> > > >
> > > > return;
> > > >
> > > > - audit_log_abend(ab, "memory violation", signr);
> > > > +
> > > > + /*Identify the reason for failure based on signal
> delivered.*/
> > > > + switch (signr) {
> > > > + case SIGABRT:
> > > > + audit_log_abend(ab, "received abort",
> signr);
> > > > + break;
> > > > + case SIGBUS:
> > > > + audit_log_abend(ab, "invalid pointer
> dereference", signr);
> > > > + break;
> > > > + case SIGFPE:
> > > > + audit_log_abend(ab, "invalid floating
> point instruction",
> > signr);
> > > > + break;
> > > > + case SIGILL:
> > > > + audit_log_abend(ab, "illegal
> instruction", signr);
> > > > + break;
> > > > + case SIGSEGV:
> > > > + audit_log_abend(ab, "memory
> violation", signr);
> > > > + break;
> > > > + case SIGTRAP:
> > > > + audit_log_abend(ab, "bad instruction /
> debugger generated
> > signal",
> > > > signr); + break;
> > > > + case SIGXCPU:
> > > > + audit_log_abend(ab, "cpu time
> violation", signr);
> > > > + break;
> > > > + case SIGXFSZ:
> > > > + audit_log_abend(ab, "file size
> violation", signr);
> > > > + break;
> > > > + default:
> > > > + audit_log_abend(ab, "not defined",
> signr);
> > > > + }
> > > >
> > > > audit_log_end(ab);
> > > >
> > > > }
> >
>
>
>
>
>
>
>
> --
> Regards,
> Paul Davies C
> vivafoss.blogspot.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fixed reason field in audit signal logging
2013-11-07 15:53 ` Eric Paris
@ 2013-11-07 16:00 ` Paul Davies C
2013-11-07 16:05 ` Eric Paris
0 siblings, 1 reply; 12+ messages in thread
From: Paul Davies C @ 2013-11-07 16:00 UTC (permalink / raw)
To: Eric Paris; +Cc: viro, linux-audit
[-- Attachment #1.1: Type: text/plain, Size: 5825 bytes --]
So we must drop the "reason" field for abnormal end due to signal delivery.
On Thu, Nov 7, 2013 at 9:23 PM, Eric Paris <eparis@redhat.com> wrote:
> On Thu, 2013-11-07 at 21:21 +0530, Paul Davies C wrote:
> > So rather than logging the "reason=memory violation" when process ends
> > abnormally due to any signal delivery , it will be be better if we
> > leave "reason=undefined" .
>
> reason=memory_violation or invalid_pointer etc
>
> although maybe it should be just 'signal'... and you can get the signal
> number from the record....
>
> > Your thoughts?
> >
> >
> > On Thu, Nov 7, 2013 at 9:12 PM, Eric Paris <eparis@redhat.com> wrote:
> > On Thu, 2013-11-07 at 10:05 -0500, Steve Grubb wrote:
> > > On Thursday, November 07, 2013 09:43:24 AM Eric Paris wrote:
> > > > On Thu, 2013-11-07 at 19:09 +0530, Paul Davies C wrote:
> > > > > The audit system logs the signals that leads to abnormal
> > end of a process.
> > > > > However , as of now , it always states the reason for
> > failure of a process
> > > > > as "memory violation" regardless of the signal
> > delivered. This is due to
> > > > > the audit_core_dumps() function pass the reason for
> > failure blindly to
> > > > > the audit_log_abend() as "memory violation".
> > > > >
> > > > > This patch changes the audit_core_dumps() function as to
> > pass on the right
> > > > > reason to the audit_log_abend based on the signal
> > received.
> > > > >
> > > > > Signed-off-by:Paul Davies C
> > > >
> > > > Acked-by: Eric Paris <eparis@redhat.com>
> > > >
> > > > But we really should wait for an Ack and thoughts from
> > steve grubb....
> > >
> > > I am confused. This is the abnormal end event I have:
> > >
> > > type=ANOM_ABEND msg=audit(1303339663.307:142): auid=4325
> > uid=0 gid=0 ses=1
> > > subj=unconfined_u:unconfined_r:unconfined_t:s0 pid=3775
> > comm="aureport" sig=11
> > >
> > > Why / when did we start adding text explanations? We should
> > not do that. We
> > > didn't have it before and it should not have been added. The
> > signal number is
> > > enough to identify the problem.
> >
> >
> > We started adding a reason when seccomp started sending
> > ANOM_ABEND
> > events as well. It doesn't do so with a signal. Agreed, the
> > " " is/was
> > a bad idea...
> >
> > >
> > > If we did need a reason= field, all these strings with
> > spaces will get
> > > separated on parsing. They should be like "memory-violation"
> > or "recieved-
> > > abort". And would it be better to hide this in the
> > audit_log_abend function? I
> > > honestly don't understand why this was added.
> > >
> > > -Steve
> > >
> > >
> > > > > ---
> > > > >
> > > > > kernel/auditsc.c | 31 ++++++++++++++++++++++++++++++-
> > > > > 1 file changed, 30 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > > index 9845cb3..3cafd13 100644
> > > > > --- a/kernel/auditsc.c
> > > > > +++ b/kernel/auditsc.c
> > > > > @@ -2395,7 +2395,36 @@ void audit_core_dumps(long signr)
> > > > >
> > > > > ab = audit_log_start(NULL, GFP_KERNEL,
> > AUDIT_ANOM_ABEND);
> > > > > if (unlikely(!ab))
> > > > >
> > > > > return;
> > > > >
> > > > > - audit_log_abend(ab, "memory violation", signr);
> > > > > +
> > > > > + /*Identify the reason for failure based on signal
> > delivered.*/
> > > > > + switch (signr) {
> > > > > + case SIGABRT:
> > > > > + audit_log_abend(ab, "received abort",
> > signr);
> > > > > + break;
> > > > > + case SIGBUS:
> > > > > + audit_log_abend(ab, "invalid pointer
> > dereference", signr);
> > > > > + break;
> > > > > + case SIGFPE:
> > > > > + audit_log_abend(ab, "invalid floating
> > point instruction",
> > > signr);
> > > > > + break;
> > > > > + case SIGILL:
> > > > > + audit_log_abend(ab, "illegal
> > instruction", signr);
> > > > > + break;
> > > > > + case SIGSEGV:
> > > > > + audit_log_abend(ab, "memory
> > violation", signr);
> > > > > + break;
> > > > > + case SIGTRAP:
> > > > > + audit_log_abend(ab, "bad instruction /
> > debugger generated
> > > signal",
> > > > > signr); + break;
> > > > > + case SIGXCPU:
> > > > > + audit_log_abend(ab, "cpu time
> > violation", signr);
> > > > > + break;
> > > > > + case SIGXFSZ:
> > > > > + audit_log_abend(ab, "file size
> > violation", signr);
> > > > > + break;
> > > > > + default:
> > > > > + audit_log_abend(ab, "not defined",
> > signr);
> > > > > + }
> > > > >
> > > > > audit_log_end(ab);
> > > > >
> > > > > }
> > >
> >
> >
> >
> >
> >
> >
> >
> > --
> > Regards,
> > Paul Davies C
> > vivafoss.blogspot.com
>
>
>
--
*Regards,*
*Paul Davies C*
vivafoss.blogspot.com
[-- Attachment #1.2: Type: text/html, Size: 8337 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fixed reason field in audit signal logging
2013-11-07 16:00 ` Paul Davies C
@ 2013-11-07 16:05 ` Eric Paris
0 siblings, 0 replies; 12+ messages in thread
From: Eric Paris @ 2013-11-07 16:05 UTC (permalink / raw)
To: Paul Davies C; +Cc: viro, linux-audit
I don't suggest dropping the reason field entirely. I thought your
patch to add signal information in the reason field was a good idea, but
steve pointed out, the signal information is in the sig= field. So now
I do not believe we need such fine grained 'reasons'
That brought me to the question is "memory violation" the right reason
when really it was a 'signal' which caused such a record... Maybe a
patch which just did s/memory violation/signal/ would be a good idea...
On Thu, 2013-11-07 at 21:30 +0530, Paul Davies C wrote:
> So we must drop the "reason" field for abnormal end due to signal
> delivery.
>
>
> On Thu, Nov 7, 2013 at 9:23 PM, Eric Paris <eparis@redhat.com> wrote:
> On Thu, 2013-11-07 at 21:21 +0530, Paul Davies C wrote:
> > So rather than logging the "reason=memory violation" when
> process ends
> > abnormally due to any signal delivery , it will be be better
> if we
> > leave "reason=undefined" .
>
>
> reason=memory_violation or invalid_pointer etc
>
> although maybe it should be just 'signal'... and you can get
> the signal
> number from the record....
>
> > Your thoughts?
> >
> >
> > On Thu, Nov 7, 2013 at 9:12 PM, Eric Paris
> <eparis@redhat.com> wrote:
> > On Thu, 2013-11-07 at 10:05 -0500, Steve Grubb
> wrote:
> > > On Thursday, November 07, 2013 09:43:24 AM Eric
> Paris wrote:
> > > > On Thu, 2013-11-07 at 19:09 +0530, Paul Davies C
> wrote:
> > > > > The audit system logs the signals that leads
> to abnormal
> > end of a process.
> > > > > However , as of now , it always states the
> reason for
> > failure of a process
> > > > > as "memory violation" regardless of the signal
> > delivered. This is due to
> > > > > the audit_core_dumps() function pass the
> reason for
> > failure blindly to
> > > > > the audit_log_abend() as "memory violation".
> > > > >
> > > > > This patch changes the audit_core_dumps()
> function as to
> > pass on the right
> > > > > reason to the audit_log_abend based on the
> signal
> > received.
> > > > >
> > > > > Signed-off-by:Paul Davies C
> > > >
> > > > Acked-by: Eric Paris <eparis@redhat.com>
> > > >
> > > > But we really should wait for an Ack and
> thoughts from
> > steve grubb....
> > >
> > > I am confused. This is the abnormal end event I
> have:
> > >
> > > type=ANOM_ABEND msg=audit(1303339663.307:142):
> auid=4325
> > uid=0 gid=0 ses=1
> > > subj=unconfined_u:unconfined_r:unconfined_t:s0
> pid=3775
> > comm="aureport" sig=11
> > >
> > > Why / when did we start adding text explanations?
> We should
> > not do that. We
> > > didn't have it before and it should not have been
> added. The
> > signal number is
> > > enough to identify the problem.
> >
> >
> > We started adding a reason when seccomp started
> sending
> > ANOM_ABEND
> > events as well. It doesn't do so with a signal.
> Agreed, the
> > " " is/was
> > a bad idea...
> >
> > >
> > > If we did need a reason= field, all these strings
> with
> > spaces will get
> > > separated on parsing. They should be like
> "memory-violation"
> > or "recieved-
> > > abort". And would it be better to hide this in the
> > audit_log_abend function? I
> > > honestly don't understand why this was added.
> > >
> > > -Steve
> > >
> > >
> > > > > ---
> > > > >
> > > > > kernel/auditsc.c | 31
> ++++++++++++++++++++++++++++++-
> > > > > 1 file changed, 30 insertions(+), 1
> deletion(-)
> > > > >
> > > > > diff --git a/kernel/auditsc.c
> b/kernel/auditsc.c
> > > > > index 9845cb3..3cafd13 100644
> > > > > --- a/kernel/auditsc.c
> > > > > +++ b/kernel/auditsc.c
> > > > > @@ -2395,7 +2395,36 @@ void
> audit_core_dumps(long signr)
> > > > >
> > > > > ab = audit_log_start(NULL, GFP_KERNEL,
> > AUDIT_ANOM_ABEND);
> > > > > if (unlikely(!ab))
> > > > >
> > > > > return;
> > > > >
> > > > > - audit_log_abend(ab, "memory violation",
> signr);
> > > > > +
> > > > > + /*Identify the reason for failure based on
> signal
> > delivered.*/
> > > > > + switch (signr) {
> > > > > + case SIGABRT:
> > > > > + audit_log_abend(ab,
> "received abort",
> > signr);
> > > > > + break;
> > > > > + case SIGBUS:
> > > > > + audit_log_abend(ab, "invalid
> pointer
> > dereference", signr);
> > > > > + break;
> > > > > + case SIGFPE:
> > > > > + audit_log_abend(ab, "invalid
> floating
> > point instruction",
> > > signr);
> > > > > + break;
> > > > > + case SIGILL:
> > > > > + audit_log_abend(ab, "illegal
> > instruction", signr);
> > > > > + break;
> > > > > + case SIGSEGV:
> > > > > + audit_log_abend(ab, "memory
> > violation", signr);
> > > > > + break;
> > > > > + case SIGTRAP:
> > > > > + audit_log_abend(ab, "bad
> instruction /
> > debugger generated
> > > signal",
> > > > > signr); + break;
> > > > > + case SIGXCPU:
> > > > > + audit_log_abend(ab, "cpu
> time
> > violation", signr);
> > > > > + break;
> > > > > + case SIGXFSZ:
> > > > > + audit_log_abend(ab, "file
> size
> > violation", signr);
> > > > > + break;
> > > > > + default:
> > > > > + audit_log_abend(ab, "not
> defined",
> > signr);
> > > > > + }
> > > > >
> > > > > audit_log_end(ab);
> > > > >
> > > > > }
> > >
> >
> >
> >
> >
> >
> >
> >
> > --
> > Regards,
> > Paul Davies C
> > vivafoss.blogspot.com
>
>
>
>
>
>
>
> --
> Regards,
> Paul Davies C
> vivafoss.blogspot.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fixed reason field in audit signal logging
2013-11-07 15:42 ` Eric Paris
2013-11-07 15:51 ` Paul Davies C
@ 2013-11-07 16:11 ` Steve Grubb
2013-11-07 18:07 ` Steve Grubb
1 sibling, 1 reply; 12+ messages in thread
From: Steve Grubb @ 2013-11-07 16:11 UTC (permalink / raw)
To: Eric Paris; +Cc: linux-audit, viro
On Thursday, November 07, 2013 10:42:21 AM Eric Paris wrote:
> > I am confused. This is the abnormal end event I have:
> >
> >
> > type=ANOM_ABEND msg=audit(1303339663.307:142): auid=4325 uid=0 gid=0
> > ses=1
> > subj=unconfined_u:unconfined_r:unconfined_t:s0 pid=3775 comm="aureport"
> > sig=11>
> >
> >
> > Why / when did we start adding text explanations? We should not do that.
> > We didn't have it before and it should not have been added. The signal
> > number is enough to identify the problem.
>
> We started adding a reason when seccomp started sending ANOM_ABEND
> events as well. It doesn't do so with a signal. Agreed, the " " is/was
> a bad idea...
Does seccomp still send these? I see there is an AUDIT_SECCOMP event being
sent by __audit_seccomp(). Does seccomp do anything with ABEND at this point?
-Steve
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fixed reason field in audit signal logging
2013-11-07 16:11 ` Steve Grubb
@ 2013-11-07 18:07 ` Steve Grubb
0 siblings, 0 replies; 12+ messages in thread
From: Steve Grubb @ 2013-11-07 18:07 UTC (permalink / raw)
To: linux-audit
On Thursday, November 07, 2013 11:11:09 AM Steve Grubb wrote:
> On Thursday, November 07, 2013 10:42:21 AM Eric Paris wrote:
> > > I am confused. This is the abnormal end event I have:
> > >
> > >
> > > type=ANOM_ABEND msg=audit(1303339663.307:142): auid=4325 uid=0 gid=0
> > > ses=1
> > > subj=unconfined_u:unconfined_r:unconfined_t:s0 pid=3775 comm="aureport"
> > > sig=11>
> > >
> > >
> > > Why / when did we start adding text explanations? We should not do that.
> > > We didn't have it before and it should not have been added. The signal
> > > number is enough to identify the problem.
> >
> > We started adding a reason when seccomp started sending ANOM_ABEND
> > events as well. It doesn't do so with a signal. Agreed, the " " is/was
> > a bad idea...
>
> Does seccomp still send these? I see there is an AUDIT_SECCOMP event being
> sent by __audit_seccomp(). Does seccomp do anything with ABEND at this
> point?
As far as I can see via grepping around, seccomp does not call
audit_log_abend(). As a matter of fact, only audit_core_dumps() does. meaning
there is no reason for audit_log_abend anymore. Its code can be pushed back
into audit_core_dumps() and the reason= can be removed entirely.
-Steve
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2013-11-07 18:07 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-07 13:39 [PATCH] Fixed reason field in audit signal logging Paul Davies C
2013-11-07 14:43 ` Eric Paris
2013-11-07 14:52 ` LC Bruzenak
2013-11-07 15:05 ` Steve Grubb
2013-11-07 15:13 ` LC Bruzenak
2013-11-07 15:42 ` Eric Paris
2013-11-07 15:51 ` Paul Davies C
2013-11-07 15:53 ` Eric Paris
2013-11-07 16:00 ` Paul Davies C
2013-11-07 16:05 ` Eric Paris
2013-11-07 16:11 ` Steve Grubb
2013-11-07 18:07 ` Steve Grubb
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox