linux-kernel - Re: [PATCH] x86/vdso/pvclock: Protect STABLE check with the seqcount

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: <CALCETrWt_A=NrwzgEsn_zV1BnejGM8fi=MgZ+mTg9kyA=2BAZg@mail.gmail.com>
Date:	Tue, 12 Jan 2016 11:48:40 -0800
From:	Andy Lutomirski <luto@...capital.net>
To:	Ingo Molnar <mingo@...nel.org>
Cc:	Andy Lutomirski <luto@...nel.org>, X86 ML <x86@...nel.org>,
	Radim Krcmar <rkrcmar@...hat.com>,
	Paolo Bonzini <pbonzini@...hat.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	kvm list <kvm@...r.kernel.org>, Alexander Graf <agraf@...e.de>,
	Marcelo Tosatti <mtosatti@...hat.com>
Subject: Re: [PATCH] x86/vdso/pvclock: Protect STABLE check with the seqcount

Hi Ingo-

Can you apply this before the tip:x86/asm pull request goes out?  It
fixes a regression in tip:x86/asm.

--Andy

On Fri, Jan 8, 2016 at 6:04 AM, Marcelo Tosatti <mtosatti@...hat.com> wrote:
> On Thu, Jan 07, 2016 at 01:13:41PM -0800, Andy Lutomirski wrote:
>> On Thu, Jan 7, 2016 at 1:02 PM, Marcelo Tosatti <mtosatti@...hat.com> wrote:
>> > On Mon, Jan 04, 2016 at 03:14:28PM -0800, Andy Lutomirski wrote:
>> >> If the clock becomes unstable while we're reading it, we need to
>> >> bail.  We can do this by simply moving the check into the seqcount
>> >> loop.
>> >>
>> >> Reported-by: Marcelo Tosatti <mtosatti@...hat.com>
>> >> Signed-off-by: Andy Lutomirski <luto@...nel.org>
>> >> ---
>> >>
>> >> Marcelo, how's this?
>> >>
>> >> arch/x86/entry/vdso/vclock_gettime.c | 12 ++++++------
>> >>  1 file changed, 6 insertions(+), 6 deletions(-)
>> >>
>> >> diff --git a/arch/x86/entry/vdso/vclock_gettime.c b/arch/x86/entry/vdso/vclock_gettime.c
>> >> index 8602f06c759f..1a50e09c945b 100644
>> >> --- a/arch/x86/entry/vdso/vclock_gettime.c
>> >> +++ b/arch/x86/entry/vdso/vclock_gettime.c
>> >> @@ -126,23 +126,23 @@ static notrace cycle_t vread_pvclock(int *mode)
>> >>        *
>> >>        * On Xen, we don't appear to have that guarantee, but Xen still
>> >>        * supplies a valid seqlock using the version field.
>> >> -
>> >> +      *
>> >>        * We only do pvclock vdso timing at all if
>> >>        * PVCLOCK_TSC_STABLE_BIT is set, and we interpret that bit to
>> >>        * mean that all vCPUs have matching pvti and that the TSC is
>> >>        * synced, so we can just look at vCPU 0's pvti.
>> >>        */
>> >>
>> >> -     if (unlikely(!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))) {
>> >> -             *mode = VCLOCK_NONE;
>> >> -             return 0;
>> >> -     }
>> >> -
>> >>       do {
>> >>               version = pvti->version;
>> >>
>> >>               smp_rmb();
>> >>
>> >> +             if (unlikely(!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))) {
>> >> +                     *mode = VCLOCK_NONE;
>> >> +                     return 0;
>> >> +             }
>> >> +
>> >>               tsc = rdtsc_ordered();
>> >>               pvti_tsc_to_system_mul = pvti->tsc_to_system_mul;
>> >>               pvti_tsc_shift = pvti->tsc_shift;
>> >> --
>> >> 2.4.3
>> >
>> > Check it before returning the value (once cleared, it can't be set back
>> > to 1), similarly to what was in place before.
>> >
>> >
>>
>> I don't understand what you mean.
>>
>> In the old code (4.3 and 4.4), the vdso checks STABLE_BIT at the end,
>> which is correct as long as STABLE_BIT can never change from 0 to 1.
>>
>> In the -tip code, it's clearly wrong.
>>
>> In the code in this patch, it should be correct regardless of how
>> STABLE_BIT changes as long as the seqcount works.  Given that the
>> performance cost of doing that is zero, I'd rather keep it that way.
>> If we're really paranoid, we could move it after the rest of the pvti
>> reads and add a barrier, but is there really any host on which that
>> matters?
>>
>> --Andy
>>
>> --
>> Andy Lutomirski
>> AMA Capital Management, LLC
>
> Right, its OK due to version check, thanks.
>



-- 
Andy Lutomirski
AMA Capital Management, LLC