3:30 a.m.
2018-06-18 17:14 GMT+02:00 Richard Guy Briggs <rgb(a)redhat.com>:
On 2018-06-18 09:35, Ondrej Mosnacek wrote:
> 2018-06-16 18:44 GMT+02:00 Richard Guy Briggs <rgb(a)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(a)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.
I think the misunderstanding is coming from the fact that
ADJ_SETOFFSET is semantically unrelated to ADJ_OFFSET_READONLY (i.e.
ADJ_OFFSET_READONLY does not necessarily mean we are not doing the
"SETOFFSET" modification). From what I understand, it is a special
value for the case when ADJ_ADJTIME is set, to override the default
behavior which always modifies the offset. ADJ_SETOFFSET is checked in
do_adjtimex (not to be confused with __do_adjtimex...) where it
actually controls setting of some different "offset" than the "other
offset" changed by ADJ_ADJTIME/ADJ_OFFSET.
It is terribly confusing, but unfortunately it is what it is...
Curiously, ADJ_ADJTIME (and its two related flags --
ADJ_OFFSET_READONLY and ADJ_OFFSET_SINGLESHOT) are defined outside of
the UAPI, but they are not referenced in the kernel anywhere outside
of the do_adjtimex() handling code... So it might be some obsolete
functionality or perhaps an undocumented API used by glibc to
implement the adjtime(3) and/or ntp_adjtime(2) functions (these don't
seem to be separate system calls, at least in the current kernel). I
haven't looked at the glibc code though, so maybe I'm pointing to the
wrong suspect here...
Either way, I would prefer the conditions to be spelled out like this,
so that the logic is easy to compare to the
timekeeping_validate_timex() function (just see where it checks for
CAP_SYS_TIME, that's when there is a non-read-only operation
happening). Right now it is quite easy to see that the logging
condition is equivalent to the CAP_SYS_TIME checks in the validation
function.
To make things even simpler, it would be perhaps worth it to modify
the timekeeping_validate_timex() function to signal whether the
operation is readonly in the return code and use that in the
condition...
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(a)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(a)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.