linux-kernel - Re: [PATCH v2 1/3] mm/secretmem: fix GUP-fast succeeding on secretmem folios

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: <ZgLiLB2Fl1lFhEYc@kernel.org>
Date: Tue, 26 Mar 2024 16:56:44 +0200
From: Mike Rapoport <rppt@...nel.org>
To: David Hildenbrand <david@...hat.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Miklos Szeredi <mszeredi@...hat.com>,
	Lorenzo Stoakes <lstoakes@...il.com>,
	xingwei lee <xrivendell7@...il.com>,
	yue sun <samsun1006219@...il.com>,
	Miklos Szeredi <miklos@...redi.hu>, stable@...r.kernel.org
Subject: Re: [PATCH v2 1/3] mm/secretmem: fix GUP-fast succeeding on
 secretmem folios

On Tue, Mar 26, 2024 at 03:32:08PM +0100, David Hildenbrand wrote:
> folio_is_secretmem() currently relies on secretmem folios being LRU folios,
> to save some cycles.
> 
> However, folios might reside in a folio batch without the LRU flag set, or
> temporarily have their LRU flag cleared. Consequently, the LRU flag is
> unreliable for this purpose.
> 
> In particular, this is the case when secretmem_fault() allocates a
> fresh page and calls filemap_add_folio()->folio_add_lru(). The folio might
> be added to the per-cpu folio batch and won't get the LRU flag set until
> the batch was drained using e.g., lru_add_drain().
> 
> Consequently, folio_is_secretmem() might not detect secretmem folios
> and GUP-fast can succeed in grabbing a secretmem folio, crashing the
> kernel when we would later try reading/writing to the folio, because
> the folio has been unmapped from the directmap.
> 
> Fix it by removing that unreliable check.
> 
> Reported-by: xingwei lee <xrivendell7@...il.com>
> Reported-by: yue sun <samsun1006219@...il.com>
> Closes: https://lore.kernel.org/lkml/CABOYnLyevJeravW=QrH0JUPYEcDN160aZFb7kwndm-J2rmz0HQ@mail.gmail.com/
> Debugged-by: Miklos Szeredi <miklos@...redi.hu>
> Tested-by: Miklos Szeredi <mszeredi@...hat.com>
> Fixes: 1507f51255c9 ("mm: introduce memfd_secret system call to create "secret" memory areas")
> Cc: <stable@...r.kernel.org>
> Signed-off-by: David Hildenbrand <david@...hat.com>

Reviewed-by: Mike Rapoport (IBM) <rppt@...nel.org>

> ---
>  include/linux/secretmem.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/secretmem.h b/include/linux/secretmem.h
> index 35f3a4a8ceb1..acf7e1a3f3de 100644
> --- a/include/linux/secretmem.h
> +++ b/include/linux/secretmem.h
> @@ -13,10 +13,10 @@ static inline bool folio_is_secretmem(struct folio *folio)
>  	/*
>  	 * Using folio_mapping() is quite slow because of the actual call
>  	 * instruction.
> -	 * We know that secretmem pages are not compound and LRU so we can
> +	 * We know that secretmem pages are not compound, so we can
>  	 * save a couple of cycles here.
>  	 */
> -	if (folio_test_large(folio) || !folio_test_lru(folio))
> +	if (folio_test_large(folio))
>  		return false;
>  
>  	mapping = (struct address_space *)
> -- 
> 2.43.2
> 

-- 
Sincerely yours,
Mike.