linux-kernel - Re: [PATCH 1/6] arm64: pgtable: Fix pte

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: <6eb6dead-4c76-d14a-dcc7-0d1411337dc6@arm.com>
Date:   Tue, 24 Nov 2020 15:32:18 +0530
From:   Anshuman Khandual <anshuman.khandual@....com>
To:     Will Deacon <will@...nel.org>, linux-kernel@...r.kernel.org
Cc:     kernel-team@...roid.com, Catalin Marinas <catalin.marinas@....com>,
        Yu Zhao <yuzhao@...gle.com>, Minchan Kim <minchan@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-mm@...ck.org, linux-arm-kernel@...ts.infradead.org,
        stable@...r.kernel.org
Subject: Re: [PATCH 1/6] arm64: pgtable: Fix pte_accessible()


On 11/20/20 8:05 PM, Will Deacon wrote:
> pte_accessible() is used by ptep_clear_flush() to figure out whether TLB
> invalidation is necessary when unmapping pages for reclaim. Although our
> implementation is correct according to the architecture, returning true
> only for valid, young ptes in the absence of racing page-table

Just curious, a PTE mapping would go into the TLB only if it is an
young one with PTE_AF bit set per the architecture ?

> modifications, this is in fact flawed due to lazy invalidation of old
> ptes in ptep_clear_flush_young() where we elide the expensive DSB
> instruction for completing the TLB invalidation.

IOW, an old PTE might have missed the required TLB invalidation via
ptep_clear_flush_young() because it's done in lazy mode. Hence just
include old valid PTEs in pte_accessible() so that TLB invalidation
could be done in ptep_clear_flush() path instead. May be TLB flush
could be done for every PTE, irrespective of its PTE_AF bit in
ptep_clear_flush_young().

> 
> Rather than penalise the aging path, adjust pte_accessible() to return
> true for any valid pte, even if the access flag is cleared.

But will not this cause more (possibly not required) TLB invalidation
in normal unmapping paths ? The cover letter mentions that this patch
fixes a real world crash. Should not the crash also be described here
in the commit message as this patch is marked for stable and has a
"Fixes: " tag.

> 
> Cc: <stable@...r.kernel.org>
> Fixes: 76c714be0e5e ("arm64: pgtable: implement pte_accessible()")
> Reported-by: Yu Zhao <yuzhao@...gle.com>
> Signed-off-by: Will Deacon <will@...nel.org>
> ---
>  arch/arm64/include/asm/pgtable.h | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index 4ff12a7adcfd..1bdf51f01e73 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -115,8 +115,6 @@ extern unsigned long empty_zero_page[PAGE_SIZE / sizeof(unsigned long)];
>  #define pte_valid(pte)		(!!(pte_val(pte) & PTE_VALID))
>  #define pte_valid_not_user(pte) \
>  	((pte_val(pte) & (PTE_VALID | PTE_USER)) == PTE_VALID)
> -#define pte_valid_young(pte) \
> -	((pte_val(pte) & (PTE_VALID | PTE_AF)) == (PTE_VALID | PTE_AF))
>  #define pte_valid_user(pte) \
>  	((pte_val(pte) & (PTE_VALID | PTE_USER)) == (PTE_VALID | PTE_USER))
>  
> @@ -126,7 +124,7 @@ extern unsigned long empty_zero_page[PAGE_SIZE / sizeof(unsigned long)];
>   * remapped as PROT_NONE but are yet to be flushed from the TLB.
>   */
>  #define pte_accessible(mm, pte)	\
> -	(mm_tlb_flush_pending(mm) ? pte_present(pte) : pte_valid_young(pte))
> +	(mm_tlb_flush_pending(mm) ? pte_present(pte) : pte_valid(pte))
>  
>  /*
>   * p??_access_permitted() is true for valid user mappings (subject to the
>