linux-kernel - Re: [PATCH 1/3] mm: Move arch_do_swap_page() call to before swap

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: <7471013e-4afb-e445-5985-2441155fc82c@redhat.com>
Date:   Sat, 13 May 2023 05:29:53 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Peter Collingbourne <pcc@...gle.com>,
        Catalin Marinas <catalin.marinas@....com>
Cc:     Qun-wei Lin (林群崴) 
        <Qun-wei.Lin@...iatek.com>, linux-arm-kernel@...ts.infradead.org,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        "surenb@...gle.com" <surenb@...gle.com>,
        Chinwen Chang (張錦文) 
        <chinwen.chang@...iatek.com>,
        "kasan-dev@...glegroups.com" <kasan-dev@...glegroups.com>,
        Kuan-Ying Lee (李冠穎) 
        <Kuan-Ying.Lee@...iatek.com>,
        Casper Li (李中榮) <casper.li@...iatek.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        vincenzo.frascino@....com,
        Alexandru Elisei <alexandru.elisei@....com>, will@...nel.org,
        eugenis@...gle.com, Steven Price <steven.price@....com>,
        stable@...r.kernel.org
Subject: Re: [PATCH 1/3] mm: Move arch_do_swap_page() call to before
 swap_free()

On 13.05.23 01:57, Peter Collingbourne wrote:
> Commit c145e0b47c77 ("mm: streamline COW logic in do_swap_page()") moved
> the call to swap_free() before the call to set_pte_at(), which meant that
> the MTE tags could end up being freed before set_pte_at() had a chance
> to restore them. One other possibility was to hook arch_do_swap_page(),
> but this had a number of problems:
> 
> - The call to the hook was also after swap_free().
> 
> - The call to the hook was after the call to set_pte_at(), so there was a
>    racy window where uninitialized metadata may be exposed to userspace.
>    This likely also affects SPARC ADI, which implements this hook to
>    restore tags.
> 
> - As a result of commit 1eba86c096e3 ("mm: change page type prior to
>    adding page table entry"), we were also passing the new PTE as the
>    oldpte argument, preventing the hook from knowing the swap index.
> 
> Fix all of these problems by moving the arch_do_swap_page() call before
> the call to free_page(), and ensuring that we do not set orig_pte until
> after the call.
> 
> Signed-off-by: Peter Collingbourne <pcc@...gle.com>
> Suggested-by: Catalin Marinas <catalin.marinas@....com>
> Link: https://linux-review.googlesource.com/id/I6470efa669e8bd2f841049b8c61020c510678965
> Cc: <stable@...r.kernel.org> # 6.1
> Fixes: ca827d55ebaa ("mm, swap: Add infrastructure for saving page metadata on swap")
> Fixes: 1eba86c096e3 ("mm: change page type prior to adding page table entry")

I'm confused. You say c145e0b47c77 changed something (which was after 
above commits), indicate that it fixes two other commits, and indicate 
"6.1" as stable which does not apply to any of these commits.

> ---
>   mm/memory.c | 26 +++++++++++++-------------
>   1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 01a23ad48a04..83268d287ff1 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3914,19 +3914,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
>   		}
>   	}
>   
> -	/*
> -	 * Remove the swap entry and conditionally try to free up the swapcache.
> -	 * We're already holding a reference on the page but haven't mapped it
> -	 * yet.
> -	 */
> -	swap_free(entry);
> -	if (should_try_to_free_swap(folio, vma, vmf->flags))
> -		folio_free_swap(folio);
> -
> -	inc_mm_counter(vma->vm_mm, MM_ANONPAGES);
> -	dec_mm_counter(vma->vm_mm, MM_SWAPENTS);
>   	pte = mk_pte(page, vma->vm_page_prot);
> -
>   	/*
>   	 * Same logic as in do_wp_page(); however, optimize for pages that are
>   	 * certainly not shared either because we just allocated them without
> @@ -3946,8 +3934,21 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
>   		pte = pte_mksoft_dirty(pte);
>   	if (pte_swp_uffd_wp(vmf->orig_pte))
>   		pte = pte_mkuffd_wp(pte);
> +	arch_do_swap_page(vma->vm_mm, vma, vmf->address, pte, vmf->orig_pte);
>   	vmf->orig_pte = pte;
>   
> +	/*
> +	 * Remove the swap entry and conditionally try to free up the swapcache.
> +	 * We're already holding a reference on the page but haven't mapped it
> +	 * yet.
> +	 */
> +	swap_free(entry);
> +	if (should_try_to_free_swap(folio, vma, vmf->flags))
> +		folio_free_swap(folio);
> +
> +	inc_mm_counter(vma->vm_mm, MM_ANONPAGES);
> +	dec_mm_counter(vma->vm_mm, MM_SWAPENTS);
> +
>   	/* ksm created a completely new copy */
>   	if (unlikely(folio != swapcache && swapcache)) {
>   		page_add_new_anon_rmap(page, vma, vmf->address);
> @@ -3959,7 +3960,6 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
>   	VM_BUG_ON(!folio_test_anon(folio) ||
>   			(pte_write(pte) && !PageAnonExclusive(page)));
>   	set_pte_at(vma->vm_mm, vmf->address, vmf->pte, pte);
> -	arch_do_swap_page(vma->vm_mm, vma, vmf->address, pte, vmf->orig_pte);
>   
>   	folio_unlock(folio);
>   	if (folio != swapcache && swapcache) {


You are moving the folio_free_swap() call after the 
folio_ref_count(folio) == 1 check, which means that such (previously) 
swapped pages that are exclusive cannot be detected as exclusive.

There must be a better way to handle MTE here.

Where are the tags stored, how is the location identified, and when are 
they effectively restored right now?

-- 
Thanks,

David / dhildenb