[PATCH v3] audit: log AUDIT_TIME_* records only from rules

[Paul Moore]

On Fri, Feb 11, 2022 at 9:15 AM Richard Guy Briggs <rgb at redhat.com> wrote:
>
> On 2022-02-10 21:08, Paul Moore wrote:
> > On Thu, Feb 10, 2022 at 6:46 PM Richard Guy Briggs <rgb at redhat.com> wrote:
> > > On 2022-01-31 20:29, Paul Moore wrote:
> > > > On Mon, Jan 31, 2022 at 6:29 PM Richard Guy Briggs <rgb at redhat.com> wrote:
> > > > > On 2022-01-31 17:02, Paul Moore wrote:
> > > > > > On Wed, Jan 26, 2022 at 8:52 AM Richard Guy Briggs <rgb at redhat.com> wrote:
> > > > > > > On 2022-01-25 22:24, Richard Guy Briggs wrote:
> >
> > This isn't as complete of a response as I would like, but I wanted to
> > get *something* back to you same-day since the delays are getting a
> > bit long ...
> >
> > > > > > [WARNING: not compiled, tested, yadda yadda]
> > > > > >
> > > > > > void audit_log_time(struct audit_context ctx, struct audit_buffer **abp)
> > > > > > {
> > > > > >   int i;
> > > > > >   int type = ctx->type;
> > > > > >   struct audit_buffer *ab = *abp;
> > > > > >   struct audit_ntp_val *ntp;
> > > > > >   const struct timespec64 *tk;
> > > > > >   const char *ntp_name[] = {
> > > > > >     "offset",
> > > > > >     "freq",
> > > > > >     "status",
> > > > > >     "tai",
> > > > > >     "tick",
> > > > > >     "adjust",
> > > > > >   };
> > > > > >
> > > > > >   do {
> > > > > >     if (type == AUDIT_TIME_ADJNTPVAL) {
> > > > > >       ntp = ctx->time.ntp_data.val;
> > > > > >       for (i = 0; i < AUDIT_NTP_NVALS; i++) {
> > > > > >         if (ntp[i].newval != ntp[i].oldval) {
> > > > > >           audit_log_format(ab,
> > > > > >             "op=%s old=%lli new=%lli",
> > > > > >             ntp_name[type],
> > > > > >             ntp[i].oldval, ntp[i].newval);
> > > > > >         }
> > > > > >       }
> > > > > >     } else {
> > > > > >       tk = &ctx->time.tk_injoffset;
> > > > > >       audit_log_format(ab, "sec=%lli nsec=%li",
> > > > > >         (long long)tk->tv_sec, tk->tv_nsec);
> > > > > >     }
> > > > > >     audit_log_end(ab);
> > > > >
> > > > > There is an audit_log_end() in the calling function, show_special(), so
> > > > > it should only be called here if there is another buffer allocated in
> > > > > this function after it.  audit_log_end() is protected should it be
> > > > > called with no valid buffer so this wouldn't create a bug.
> > > >
> > > > As audit_log_end() can safely take a NULL audit_buffer I don't care if we
> > > > send it back a valid buffer or a NULL.  IMO it happens to be easier (and
> > > > cleaner) to send back a NULL.
> > >
> > > None of the other callees in show_special() do that, so this would be
> > > surprising behaviour that could cause a future bug ...
> >
> > To be both honest and frank: I disagree with your assessment.  If you
> > really want to be concerned about this, there are plenty of ways to
> > mitigate the "risk" depending on your comfort level; comments and
> > returning within the switch/case block are just some of the options.
> >
> > > > > >     if (*abp) {
> > > > > >       *abp = NULL;
> > > > > >       type = (type == AUDIT_TIME_ADJNTPVAL ?
> > > > > >         AUDIT_TIME_INJOFFSET : AUDIT_TIME_ADJNTPVAL);
> > > > >
> > > > > This cannot be allocated if there are no more needed above ...
> > > >
> > > > My mistake, I was distracted a few times while typing up my reply and the
> > > > code within; while I had that detail in mind when I started I lost it
> > > > during one of the interruptions.  As penance, I wrote up some slightly more
> > > > proper code and at least made sure it complied, although I've not tried
> > > > booting it ...
> > >
> > > Did you test the code I submitted?  It compiles and works.  I found this
> > > code harder to follow.  This was partly why I wanted to leave one of the
> > > record types outside of show_special() but I did find a way to
> > > accomodate both with a minimum of overhead.
> >
> > Once again, I disagree with your assessment of the code.  I'm not sure
> > how to put this politely, but I personally found your audit_log_time()
> > implementation to be awkward; the AUDIT_TIME_INJOFFSET goto/jump to
> > "tk" at the top of the function was not something I'm going to merge.
> > You don't have to like my suggestion, but please send me a patch that
> > has a somewhat reasonable code flow.  I know you often want, or at
> > least ask for explicit suggestions (hence my untested code example),
> > but since you didn't like that let me just say this: when in doubt,
> > limit your use of gotos/jumps to error handling unless there is
> > something significantly unusual about the function.  In my opinion
> > there is nothing significantly unusual about the audit_log_time()
> > function to require a jump as you've currently done.
>
> I hate gotos.  I first learned to program on Waterloo Structured BASIC
> 4.0 on a Commodore PET 2001 where the language structure provided the
> tools to be able to avoid them.  I've avoided them and reluctantly used
> them at your urging, so now I'm a bit confused.

If you're confused follow the recommendations at the end of the
paragraph directly above your reply.  Or look at the sample code I
proposed.  You've been contributing long enough that this shouldn't be
that hard.

-- 
paul-moore.com