* [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