Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20180824120001.20771-1-omosnace@redhat.com> <20180824120001.20771-2-omosnace@redhat.com>
 <CAHC9VhTZkrmgt7rxNYWTgYvm+TVGgRD6iFu9dDtZ7eYNXBKqHQ@mail.gmail.com>
In-Reply-To: <CAHC9VhTZkrmgt7rxNYWTgYvm+TVGgRD6iFu9dDtZ7eYNXBKqHQ@mail.gmail.com>
From:   Ondrej Mosnacek <omosnace@redhat.com>
Date:   Mon, 17 Sep 2018 14:38:19 +0200
Message-ID: <CAFqZXNuir-8TE7WUHYhw5t446THO=sCjOXv+WAxNDs79e6OTWQ@mail.gmail.com>
Subject: Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments
To:     Paul Moore <paul@paul-moore.com>
Cc:     Linux-Audit Mailing List <linux-audit@redhat.com>,
        Richard Guy Briggs <rgb@redhat.com>,
        Steve Grubb <sgrubb@redhat.com>,
        Miroslav Lichvar <mlichvar@redhat.com>,
        John Stultz <john.stultz@linaro.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Stephen Boyd <sboyd@kernel.org>,
        Linux kernel mailing list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Sep 14, 2018 at 5:19 AM Paul Moore <paul@paul-moore.com> wrote:
> On Fri, Aug 24, 2018 at 8:00 AM Ondrej Mosnacek <omosnace@redhat.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@redhat.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...)

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

>
> --
> paul moore
> www.paul-moore.com--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.