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 for Android: free password hash cracker in your pocket

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

Date:	Thu, 7 Jan 2016 13:13:41 -0800
From:	Andy Lutomirski <luto@...capital.net>
To:	Marcelo Tosatti <mtosatti@...hat.com>
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>
Subject: Re: [PATCH] x86/vdso/pvclock: Protect STABLE check with the seqcount

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