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

From: Steve Grubb <sgrubb@redhat.com>
To: Richard Guy Briggs <rgb@redhat.com>
Cc: Paul Moore <paul@paul-moore.com>,
	omosnace@redhat.com, linux-audit@redhat.com, mlichvar@redhat.com,
	john.stultz@linaro.org, tglx@linutronix.de, sboyd@kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments
Date: Fri, 14 Sep 2018 11:34:00 -0400	[thread overview]
Message-ID: <2062051.Ftk13INWii@x2> (raw)
In-Reply-To: <20180914151643.gwvm5te4nvion5ex@madcap2.tricolour.ca>

On Friday, September 14, 2018 11:16:43 AM EDT Richard Guy Briggs wrote:
> On 2018-09-13 23:18, Paul Moore 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.
> > 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?
> 
> Why not do something like:
> 
> 	 type=TIME_CHANGE var=<var> new=<value_new> old=<value_old>
> 
> So that we don't pollute the field namespace *and* create 8 variants on
> the same record format?  This shouldn't be much of a concern with binary
> record formats, but we're stuck with the current parsing scheme for now.

Something like this or the other format is fine. Neither hurt parsing because 
these are not searchable fields. We only have issues when it involves a 
searchable field name.

HTH...

-Steve

> > >         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
> > * https://github.com/linux-audit/audit-kernel/wiki
> 
> - RGB
> 
> --
> Richard Guy Briggs <rgb@redhat.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