Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments

[Paul Moore]

On Mon, Sep 17, 2018 at 8:38 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
>
> On Fri, Sep 14, 2018 at 5:19 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > On Fri, Aug 24, 2018 at 8:00 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> > > This patch adds two auxiliary record types that will be used to annotate
> > > the adjtimex SYSCALL records with the NTP/timekeeping values that have
> > > been changed.
> > >
> > > Next, it adds two functions to the audit interface:
> > >  - audit_tk_injoffset(), which will be called whenever a timekeeping
> > >    offset is injected by a syscall from userspace,
> > >  - audit_ntp_adjust(), which will be called whenever an NTP internal
> > >    variable is changed by a syscall from userspace.
> > >
> > > Quick reference for the fields of the new records:
> > >     AUDIT_TIME_INJOFFSET
> > >         sec - the 'seconds' part of the offset
> > >         nsec - the 'nanoseconds' part of the offset
> > >     AUDIT_TIME_ADJNTPVAL
> > >         op - which value was adjusted:
> > >             offset - corresponding to the time_offset variable
> > >             freq   - corresponding to the time_freq variable
> > >             status - corresponding to the time_status variable
> > >             adjust - corresponding to the time_adjust variable
> > >             tick   - corresponding to the tick_usec variable
> > >             tai    - corresponding to the timekeeping's TAI offset
> >
> > I understand that reusing "op" is tempting, but the above aren't
> > really operations, they are state variables which are being changed.
>
> I remember Steve (or was it Richard?) convincing me at one of the
> meetings that "op" is the right filed name to use, despite it not
> being a name for an operation... But I don't really care, I'm okay
> with changing it to e.g. "var" as Richard suggests later in this
> thread.

As I said before, this seems like an abuse of the "op" field.

> > Using the CONFIG_CHANGE record as a basis, I wonder if we are better
> > off with something like the following:
> >
> >  type=TIME_CHANGE <var>=<value_new> old=<value_old>
> >
> > ... you might need to preface the variable names with something like
> > "ntp_" or "offset_".  You'll notice I'm also suggesting we use a
> > single record type here; is there any reason why two records types are
> > required?
>
> There are actually two reasons:
> 1. The injected offset is a timespec64, so it consists of two integer
> values (and it would be weird to produce two records for it, since IMO
> it is conceptually still a single variable).
> 2. In all other cases the variable is reset to the (possibly
> transformed) input value, while in this case the input value is added
> directly to the system time. This can be viewed as a kind of variable
> too, but it would be weird to report old and new value for it, since
> its value flows with time.
>
> Plus, when I look at:
> type=TIME_INJOFFSET [...]: sec=-16 nsec=124887145
>
> I can immediately see that the time was shifted back by 16-something
> seconds, while when I look at something like:
>
> type=TIME_CHANGE [...]: var=time_sec new=1537185685 old=1537185701
> type=TIME_CHANGE [...]: var=time_nsec new=664373417 old=789260562
>
> I can just see some big numbers that I need to do math with before I
> get an idea of what is the magnitude (or sign) of the change.

Okay, with that in mind, perhaps when recording the offset values we
omit the "old" values (arguably that doesn't make much sense here) and
keep the sec/nsec split:

type=TIME_CHANGE [...]: offset_sec=<X> offset_nsec=<Y>

... and for all others we stick with:

type=TIME_CHANGE [...]: ntp_<VAR>=<NEWVAL> old=<OLD_VAL>

... and if that results in multiple TIME_CHANGE records for a given
event, that's fine with me.

> > A reminder that we need tests for these new records and a RFE page on the wiki:
> >
> > * https://github.com/linux-audit/audit-testsuite
>
> I was going to start working on this once the format issues are
> settled. (Although I probably should have kept the RFC in the subject
> until then...)
>
> > * https://github.com/linux-audit/audit-kernel/wiki
>
> I admit I forgot about this duty, but again I would like to wait for
> the discussions to settle before writing that up.

That is fine, do it in whatever order works best for you, just
understand that I'm probably not going to merge patches like this
until I see both documentation and tests.

-- 
paul moore
www.paul-moore.com