linux-kernel - Re: [PATCH 1/1] page cache: fix page_cache_next/prev

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: <20230602175920.4891c718afd2b20b7cd620cb@linux-foundation.org>
Date:   Fri, 2 Jun 2023 17:59:20 -0700
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     Mike Kravetz <mike.kravetz@...cle.com>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        linux-fsdevel@...r.kernel.org,
        Matthew Wilcox <willy@...radead.org>,
        Ackerley Tng <ackerleytng@...gle.com>,
        Sidhartha Kumar <sidhartha.kumar@...cle.com>,
        Muchun Song <songmuchun@...edance.com>, vannapurve@...gle.com,
        erdemaktas@...gle.com
Subject: Re: [PATCH 1/1] page cache: fix page_cache_next/prev_miss off by
 one

On Fri,  2 Jun 2023 15:57:47 -0700 Mike Kravetz <mike.kravetz@...cle.com> wrote:

> Ackerley Tng reported an issue with hugetlbfs fallocate here[1].  The
> issue showed up after the conversion of hugetlb page cache lookup code
> to use page_cache_next_miss.

So I'm assuming

Fixes: d0ce0e47b323 ("mm/hugetlb: convert hugetlb fault paths to use alloc_hugetlb_folio()")

?

> Code in hugetlb fallocate, userfaultfd
> and GUP is now using page_cache_next_miss to determine if a page is
> present the page cache.  The following statement is used.
> 
> 	present = page_cache_next_miss(mapping, index, 1) != index;
> 
> There are two issues with page_cache_next_miss when used in this way.
> 1) If the passed value for index is equal to the 'wrap-around' value,
>    the same index will always be returned.  This wrap-around value is 0,
>    so 0 will be returned even if page is present at index 0.
> 2) If there is no gap in the range passed, the last index in the range
>    will be returned.  When passed a range of 1 as above, the passed
>    index value will be returned even if the page is present.
> The end result is the statement above will NEVER indicate a page is
> present in the cache, even if it is.
> 
> As noted by Ackerley in [1], users can see this by hugetlb fallocate
> incorrectly returning EEXIST if pages are already present in the file.
> In addition, hugetlb pages will not be included in core dumps if they
> need to be brought in via GUP.  userfaultfd UFFDIO_COPY also uses this
> code and will not notice pages already present in the cache.  It may try
> to allocate a new page and potentially return ENOMEM as opposed to
> EEXIST.
> 
> Both page_cache_next_miss and page_cache_prev_miss have similar issues.
> Fix by:
> - Check for index equal to 'wrap-around' value and do not exit early.
> - If no gap is found in range, return index outside range.
> - Update function description to say 'wrap-around' value could be
>   returned if passed as index.
> 
> [1] https://lore.kernel.org/linux-mm/cover.1683069252.git.ackerleytng@google.com/
>