linux-kernel - RE: [PATCH] Linux VM workaround for Knights Landing A/D leak

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: <C1C2579D7BE026428F81F41198ADB17237A866C6@irsmsx110.ger.corp.intel.com>
Date:	Wed, 15 Jun 2016 13:06:17 +0000
From:	"Anaczkowski, Lukasz" <lukasz.anaczkowski@...el.com>
To:	Dave Hansen <dave.hansen@...ux.intel.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	"tglx@...utronix.de" <tglx@...utronix.de>,
	"mingo@...hat.com" <mingo@...hat.com>,
	"ak@...ux.intel.com" <ak@...ux.intel.com>,
	"kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
	"mhocko@...e.com" <mhocko@...e.com>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"hpa@...or.com" <hpa@...or.com>
CC:	"Srinivasappa, Harish" <harish.srinivasappa@...el.com>,
	"Odzioba, Lukasz" <lukasz.odzioba@...el.com>
Subject: RE: [PATCH] Linux VM workaround for Knights Landing A/D leak

From: Dave Hansen [mailto:dave.hansen@...ux.intel.com] 
Sent: Tuesday, June 14, 2016 7:20 PM

>> diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
...
>> +extern void fix_pte_leak(struct mm_struct *mm, unsigned long addr,
>> +			 pte_t *ptep);

> Doesn't hugetlb.h somehow #include pgtable.h?  So why double-define
> fix_pte_leak()?

It's other way round - pgtable.h somehow includes hugetlb.h. I've removed
duplicated fix_pte_leak() declaration.

>> diff --git a/arch/x86/include/asm/pgtable_64.h b/arch/x86/include/asm/pgtable_64.h
>> index 2ee7811..6fa4079 100644
>> --- a/arch/x86/include/asm/pgtable_64.h
>> +++ b/arch/x86/include/asm/pgtable_64.h
>> @@ -178,6 +178,12 @@ extern void cleanup_highmap(void);
>>  extern void init_extra_mapping_uc(unsigned long phys, unsigned long size);
>>  extern void init_extra_mapping_wb(unsigned long phys, unsigned long size);
>>  
>> +#define ARCH_HAS_NEEDS_SWAP_PTL 1
>> +static inline bool arch_needs_swap_ptl(void)
>> +{
>> +       return boot_cpu_has_bug(X86_BUG_PTE_LEAK);
>> +}
>> +
>>  #endif /* !__ASSEMBLY__ */

> I think we need a comment on this sucker.  I'm not sure we should lean
> solely on the commit message to record why we need this until the end of
> time.

OK.

>> +	if (c->x86_model == 87) {

> Please use the macros in here for the model id:

OK.

> http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/tree/arch/x86/include/asm/intel-family.h

> We also probably want to prefix the pr_info() with something like
> "x86/intel:".

OK

>> +/*
>> + * Workaround for KNL issue:

> Please be specific about what this "KNL issue" *is*. 

OK

>> + * A thread that is going to page fault due to P=0, may still
>> + * non atomically set A or D bits, which could corrupt swap entries.
>> + * Always flush the other CPUs and clear the PTE again to avoid
>> + * this leakage. We are excluded using the pagetable lock.
>> + */
>> +
>> +void fix_pte_leak(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
>> +{
>> +	if (cpumask_any_but(mm_cpumask(mm), smp_processor_id()) < nr_cpu_ids) {
>> +		trace_tlb_flush(TLB_LOCAL_SHOOTDOWN, TLB_FLUSH_ALL);
>> +		flush_tlb_others(mm_cpumask(mm), mm, addr,
>> +				 addr + PAGE_SIZE);
>> +		mb();
>> +		set_pte(ptep, __pte(0));
>> +	}
>> +}
>
> I think the comment here is a bit sparse.  Can we add some more details,
> like:
>
>	Entering here, the current CPU just cleared the PTE.  But,
>	another thread may have raced and set the A or D bits, or be
>	_about_ to set the bits.  Shooting their TLB entry down will
>	ensure they see the cleared PTE and will not set A or D.
>
> and by the set_pte():
>
>	Clear the PTE one more time, in case the other thread set A/D
>	before we sent the TLB flush.

Thanks,
Lukasz