Messages in this thread | | | From | Ondrej Mosnacek <> | Date | Fri, 21 Sep 2018 13:21:22 +0200 | Subject | Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments |
| |
On Mon, Sep 17, 2018 at 4:51 PM Paul Moore <paul@paul-moore.com> wrote: > On Mon, Sep 17, 2018 at 8:38 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > 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. > > 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>
Alright, that format would work. However, I would still like to have a separate type for the offset injection, since it has different field structure and semantics (difference vs. new+old). I don't see any reason to sacrifice the distinction for just one record type slot (AFAIK we technically still have about 2 billion left...).
(Maybe you just duplicated the record type by mistake, in that case please disregard the last sentence.)
> > ... 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
-- Ondrej Mosnacek <omosnace at redhat dot com> Associate Software Engineer, Security Technologies Red Hat, Inc.
| |