linux-kernel - Re: [RFC PATCH ghak10 v6 1/2] timekeeping: Audit clock adjustments

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFqZXNtHxt6Huv2ufU-gp-0Y4WfyQSJkYVS0cmd0o+G=D1A-UQ@mail.gmail.com>
Date:   Mon, 1 Apr 2019 11:16:48 +0200
From:   Ondrej Mosnacek <omosnace@...hat.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Linux-Audit Mailing List <linux-audit@...hat.com>,
        Paul Moore <paul@...l-moore.com>,
        Richard Guy Briggs <rgb@...hat.com>,
        Steve Grubb <sgrubb@...hat.com>,
        Miroslav Lichvar <mlichvar@...hat.com>,
        John Stultz <john.stultz@...aro.org>,
        Stephen Boyd <sboyd@...nel.org>,
        Linux kernel mailing list <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH ghak10 v6 1/2] timekeeping: Audit clock adjustments

On Thu, Mar 28, 2019 at 1:09 AM Thomas Gleixner <tglx@...utronix.de> wrote:
> On Thu, 7 Mar 2019, Ondrej Mosnacek wrote:
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -2512,6 +2512,14 @@ void __audit_fanotify(unsigned int response)
> >               AUDIT_FANOTIFY, "resp=%u", response);
> >  }
> >
> > +/* We need to allocate with GFP_ATOMIC here, since these two functions will be
> > + * called while holding the timekeeping lock: */
>
> Audit is no justification for doing ATOMIC allocations just because it's
> convenient in the middle of code which blocks every concurrent reader.
>
> Please find a place outside of the timekeeper lock to do that audit
> logging. Either that or allocate your buffer upfront in a preemptible
> section and commit after the critical section.
>
> /*
>  * Aside of that please use proper multiline comment style and not this
>  * horrible other one.
>  */

Oh, sorry, I wrote that code last summer, when I didn't quite have the
kernel coding style in my blood yet :) But fortunately I shouldn't
need that comment at all in the next version...

>
> > +void __audit_tk_injoffset(struct timespec64 offset)
> > +{
> > +     audit_log(audit_context(), GFP_ATOMIC, AUDIT_TIME_INJOFFSET,
> > +               "sec=%lli nsec=%li", (long long)offset.tv_sec, offset.tv_nsec);
> > +}
> > +
> > @@ -1250,6 +1251,9 @@ out:
> >       /* signal hrtimers about time change */
> >       clock_was_set();
> >
> > +     if (!ret)
> > +             audit_tk_injoffset(ts_delta);
>
> This one does not need GFP_ATOMIC at all.
>
> > +
> >       return ret;
> >  }
> >  EXPORT_SYMBOL(do_settimeofday64);
> > @@ -2322,6 +2326,8 @@ int do_adjtimex(struct timex *txc)
> >               ret = timekeeping_inject_offset(&delta);
> >               if (ret)
> >                       return ret;
> > +
> > +             audit_tk_injoffset(delta);
> >       }
> >
> >       ktime_get_real_ts64(&ts);
>
> This can be done at the end of do_adjtimex() quite nicely in preemptible
> context.

But wait, isn't this call outside of the critical section as well? (I
must have been moving the call around when I was writing the code and
didn't realize that this function actually doesn't need GFP_ATOMIC at
all...) Or am I missing something?


--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.