[RFC PATCH ghak10 3/3] ntp: Audit valid attempts to adjust the clock

[Richard Guy Briggs]

On 2018-06-18 09:35, Ondrej Mosnacek wrote:
> 2018-06-16 18:44 GMT+02:00 Richard Guy Briggs <rgb at redhat.com>:
> > On 2018-06-15 14:45, Ondrej Mosnacek wrote:
> >> (Intentionally not sending to the timekeeping/ntp maintainers just yet,
> >> let's settle on the record contents/format first.)
> >>
> >> Signed-off-by: Ondrej Mosnacek <omosnace at redhat.com>
> >> ---
> >>  kernel/time/ntp.c | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >>
> >> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
> >> index a09ded765f6c..78a01df0dbdb 100644
> >> --- a/kernel/time/ntp.c
> >> +++ b/kernel/time/ntp.c
> >> @@ -18,6 +18,7 @@
> >>  #include <linux/module.h>
> >>  #include <linux/rtc.h>
> >>  #include <linux/math64.h>
> >> +#include <linux/audit.h>
> >>
> >>  #include "ntp_internal.h"
> >>  #include "timekeeping_internal.h"
> >> @@ -722,6 +723,16 @@ int __do_adjtimex(struct timex *txc, struct timespec64 *ts, s32 *time_tai)
> >>  {
> >>       int result;
> >>
> >> +     /* Only log audit event if the clock was changed/attempted to be changed.
> >> +      * Based on the logic inside timekeeping_validate_timex().
> >> +      * NOTE: We need to log the event before any of the fields get
> >> +      * overwritten by the output values (the function will not fail, so it
> >> +      * is OK). */
> >> +     if (   ( (txc->modes & ADJ_ADJTIME) && !(txc->modes & ADJ_OFFSET_READONLY))
> >> +         || (!(txc->modes & ADJ_ADJTIME) &&   txc->modes)
> >> +         ||   (txc->modes & ADJ_SETOFFSET))
> >
> > Wouldn't the third condition be covered by the second?
> 
> Not if txc->modes has ADJ_ADJTIME set, ADJ_OFFSET_READONLY unset, and
> ADJ_SETOFFSET set. Then it fails the first two conditions and only
> matches on the third one. I don't know if such combination can
> actually occur in practice, but timekeeping_validate_timex() doesn't
> reject it, so it's better to be on the safe side here.

But the second condition would trigger on ADJ_OFFSET_READONLY when
ADJ_ADJTIME is not set (which may never happen together), which includes
ADJ_SETOFFSET alone or any other flag.

Ok, for the second condition how about: txc->modes & ~(ADJ_ADJTIME | ADJ_OFFSET_READONLY)

Or better yet if you only care about ADJ_ADJTIME and ADJ_SETOFFSET, how about only:
	txc->modes & (ADJ_ADJTIME | ADJ_SETOFFSET) && txc->modes != ADJ_OFFSET_READONLY
but I don't think that is what you want.

> BTW, I just realized that the logging function can be called directly
> from inside the do_adjtimex() function in timekeeping.c (which is a
> wrapper around __do_adjtimex()), where it probably suits better (since
> this function contains the ADJ_SETOFFSET handling code and
> timekeeping.c also contains the timekeeping_validate_timex() function
> referenced in the comment). I will move it in v2.
> 
> >
> >> +             audit_adjtime(txc);
> >> +
> >>       if (txc->modes & ADJ_ADJTIME) {
> >>               long save_adjust = time_adjust;
> >
> > - RGB
> >
> > --
> > Richard Guy Briggs <rgb at 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
> 
> -- 
> Ondrej Mosnacek <omosnace at redhat dot com>
> Associate Software Engineer, Security Technologies
> Red Hat, Inc.

- RGB

--
Richard Guy Briggs <rgb at 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