Re: [PATCH] Fixed reason field in audit signal logging

[Eric Paris <eparis@redhat.com>]

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