linux-kernel - Re: [PATCH 1/2] time: x86: Fix race switching from vsyscall to non-vsyscall clock

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: <CALCETrWheuZGrMYzvWDRoRjNJt5mD-0FEd+PQRh3N=XjfL98gQ@mail.gmail.com>
Date:	Wed, 14 Mar 2012 18:43:19 -0700
From:	Andy Lutomirski <luto@...capital.net>
To:	John Stultz <john.stultz@...aro.org>
Cc:	Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] time: x86: Fix race switching from vsyscall to
 non-vsyscall clock

On Wed, Mar 14, 2012 at 5:42 PM, John Stultz <john.stultz@...aro.org> wrote:
> On 03/14/2012 05:34 PM, Thomas Gleixner wrote:
>>
>> On Wed, 14 Mar 2012, John Stultz wrote:
>>>
>>>  notrace static noinline int do_realtime(struct timespec *ts)
>>>  {
>>>        unsigned long seq, ns;
>>> +       int mode;
>>
>> Please keep a newline between declarations and code.
>
>
> Fixed below. Thanks!
> (Let me know if you see whitespace damage, I switched mail clients today and
> am learning the quirks here.)
> -john
>
>
>
> When switching from a vsyscall capable to a non-vsyscall capable
> clocksource, there was a small race, where the last vsyscall
> gettimeofday before the switch might return a invalid time value
> using the new non-vsyscall enabled clocksource values after the
> switch is complete.
>
> This is due to the vsyscall code checking the vclock_mode once
> outside of the seqcount protected section. After it reads the
> vclock mode, it doesn't re-check that the sampled clock data
> that is obtained in the seqcount critical section still matches.
>
> The fix is to sample vclock_mode inside the protected section,
> and as long as it isn't VCLOCK_NONE, return the calculated
> value. If it has changed and is now VCLOCK_NONE, fall back
> to the syscall gettime calculation.
>
> v2:
>  * Cleanup checks as suggested by tglx
>  * Also fix same issue present in gettimeofday path
>
> CC: Andy Lutomirski<luto@...capital.net>
> CC: Thomas Gleixner<tglx@...utronix.de>
> Signed-off-by: John Stultz<john.stultz@...aro.org>
> ---
>  arch/x86/vdso/vclock_gettime.c |   68
> +++++++++++++++++++++++++--------------
>  1 files changed, 43 insertions(+), 25 deletions(-)
>

Looks reasonable to me.  I like this approach better than the earlier
way -- it's likely to cause less slowdown in the VCLOCK_TSC case.

That being said, I think you might have a bug:

notrace static inline long vgetns(void)
{
        long v;
        cycles_t cycles;
        if (gtod->clock.vclock_mode == VCLOCK_TSC)
                cycles = vread_tsc();
        else
                cycles = vread_hpet();
        v = (cycles - gtod->clock.cycle_last) & gtod->clock.mask;
        return (v * gtod->clock.mult) >> gtod->clock.shift;
}

In the VCLOCK_NONE, you'll access the hpet mapping.  But in
hpet_enable, hpet_set_mapping isn't called and this will crash, I
think.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/