Message-ID: <20180917142000.akpg7ul4jufujo4o@madcap2.tricolour.ca>
Date:   Mon, 17 Sep 2018 10:20:00 -0400
From:   Richard Guy Briggs <rgb@...hat.com>
To:     Ondrej Mosnacek <omosnace@...hat.com>
Cc:     Paul Moore <paul@...l-moore.com>,
        Linux-Audit Mailing List <linux-audit@...hat.com>,
        Steve Grubb <sgrubb@...hat.com>,
        Miroslav Lichvar <mlichvar@...hat.com>,
        John Stultz <john.stultz@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Stephen Boyd <sboyd@...nel.org>,
        Linux kernel mailing list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time
 adjustments

On 2018-09-17 14:38, Ondrej Mosnacek wrote:
> On Fri, Sep 14, 2018 at 5:19 AM Paul Moore <paul@...l-moore.com> wrote:
> > On Fri, Aug 24, 2018 at 8:00 AM Ondrej Mosnacek <omosnace@...hat.com> 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.
> 
> > 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.
> 
> > >         old - the old value
> > >         new - the new value
> > >
> > > Signed-off-by: Ondrej Mosnacek <omosnace@...hat.com>
> > > ---
> > >  include/linux/audit.h      | 21 +++++++++++++++++++++
> > >  include/uapi/linux/audit.h |  2 ++
> > >  kernel/auditsc.c           | 15 +++++++++++++++
> > >  3 files changed, 38 insertions(+)
> >
> > 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...)

There are more iterative development methods (to which we appear to be
trying to steer) where the test is written first, partly helping clarify
what the goal is.  Refining the test is incremental.

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

While tempting to leave it to the end, documenting it initially can help
clarify in your own mind what you are trying to accomplish and can help
others understand what you are trying to do.

> > paul moore

> Ondrej Mosnacek <omosnace at redhat dot com>

- RGB

--
Richard Guy Briggs <rgb@...hat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Powered by blists - more mailing lists