linux-kernel - Re: [PATCH] Fix page corruption caused by racy check in __free

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: <Y+VBIMEKI1BiSu4X@casper.infradead.org>
Date:   Thu, 9 Feb 2023 18:53:20 +0000
From:   Matthew Wilcox <willy@...radead.org>
To:     David Chen <david.chen@...anix.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH] Fix page corruption caused by racy check in __free_pages

On Thu, Feb 09, 2023 at 05:48:28PM +0000, David Chen wrote:
> So the problem is the check PageHead is racy because at this point we
> already dropped our reference to the page. So even if we came in with
> compound page, the page can already be freed and PageHead can return
> false and we will end up freeing all the tail pages causing double free.
> 
> Fixes: e320d3012d25 ("mm/page_alloc.c: fix freeing non-compound pages")
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Matthew Wilcox (Oracle) <willy@...radead.org>
> Cc: linux-mm@...ck.org
> Cc: stable@...r.kernel.org
> Signed-off-by: Chunwei Chen <david.chen@...anix.com>

Reviewed-by: Matthew Wilcox (Oracle) <willy@...radead.org>

That's a pretty narrow window you managed to hit!  Just a few
instructions wide.  I guess that answers the question Andrew originally
had (Does this ever happen?)  I just changed it from a silent memory
leak into a double-free.

> ---
>  mm/page_alloc.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 0745aedebb37..3bb3484563ed 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5631,9 +5631,12 @@ EXPORT_SYMBOL(get_zeroed_page);
>   */
>  void __free_pages(struct page *page, unsigned int order)
>  {
> +	/* get PageHead before we drop reference */
> +	int head = PageHead(page);
> +
>  	if (put_page_testzero(page))
>  		free_the_page(page, order);
> -	else if (!PageHead(page))
> +	else if (!head)
>  		while (order-- > 0)
>  			free_the_page(page + (1 << order), order);
>  }
> -- 
> 2.22.3