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

2018-06-16 18:44 GMT+02:00 Richard Guy Briggs <rgb(a)redhat.com&gt;: > 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(a)redhat.com&gt; >> --- >> 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.

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(a)redhat.com&gt; > 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.