On Fri, Mar 22, 2024 at 10:56:08PM +0100, David Hildenbrand wrote:
> On 22.03.24 22:37, David Hildenbrand wrote:
> > On 22.03.24 22:33, David Hildenbrand wrote:
> > > On 22.03.24 22:18, David Hildenbrand wrote:
> > > > On 22.03.24 22:13, Miklos Szeredi wrote:
> > > > > On Fri, 22 Mar 2024 at 22:08, David Hildenbrand <david@xxxxxxxxxx> wrote:
> > > > > >
> > > > > > On 22.03.24 20:46, Miklos Szeredi wrote:
> > > > > > > On Fri, 22 Mar 2024 at 16:41, David Hildenbrand <david@xxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > > But at least the vmsplice() just seems to work. Which is weird, because
> > > > > > > > GUP-fast should not apply (page not faulted in?)
> > > > > > >
> > > > > > > But it is faulted in, and that indeed seems to be the root cause.
> > > > > >
> > > > > > secretmem mmap() won't populate the page tables. So it's not faulted in yet.
> > > > > >
> > > > > > When we GUP via vmsplice, GUP-fast should not find it in the page tables
> > > > > > and fallback to slow GUP.
> > > > > >
> > > > > > There, we seem to pass check_vma_flags(), trigger faultin_page() to
> > > > > > fault it in, and then find it via follow_page_mask().
> > > > > >
> > > > > > ... and I wonder how we manage to skip check_vma_flags(), or otherwise
> > > > > > managed to GUP it.
> > > > > >
> > > > > > vmsplice() should, in theory, never succeed here.
> > > > > >
> > > > > > Weird :/
> > > > > >
> > > > > > > Improved repro:
> > > > > > >
> > > > > > > #define _GNU_SOURCE
> > > > > > >
> > > > > > > #include <fcntl.h>
> > > > > > > #include <unistd.h>
> > > > > > > #include <stdio.h>
> > > > > > > #include <errno.h>
> > > > > > > #include <sys/mman.h>
> > > > > > > #include <sys/syscall.h>
> > > > > > >
> > > > > > > int main(void)
> > > > > > > {
> > > > > > > int fd1, fd2;
> > > > > > > int pip[2];
> > > > > > > struct iovec iov;
> > > > > > > char *addr;
> > > > > > > int ret;
> > > > > > >
> > > > > > > fd1 = syscall(__NR_memfd_secret, 0);
> > > > > > > addr = mmap(NULL, 4096, PROT_READ | PROT_WRITE, MAP_SHARED, fd1, 0);
> > > > > > > ftruncate(fd1, 7);
> > > > > > > addr[0] = 1; /* fault in page */
> > > > >
> > > > > Here the page is faulted in and GUP-fast will find it. It's not in
> > > > > the kernel page table, but it is in the user page table, which is what
> > > > > matter for GUP.
> > > >
> > > > Trust me, I know the GUP code very well :P
> > > >
> > > > gup_pte_range -- GUP fast -- contains:
> > > >
> > > > if (unlikely(folio_is_secretmem(folio))) {
> > > > gup_put_folio(folio, 1, flags);
> > > > goto pte_unmap;
> > > > }
> > > >
> > > > So we "should" be rejecting any secretmem folios and fallback to GUP slow.
> > > >
> > > >
> > > > ... we don't check the same in gup_huge_pmd(), but we shouldn't ever see
> > > > THP in secretmem code.
> > > >
> > >
> > > Ehm:
> > >
> > > [ 29.441405] Secretmem fault: PFN: 1096177
> > > [ 29.442092] GUP-fast: PFN: 1096177
> > >
> > >
> > > ... is folio_is_secretmem() broken?
> > >
> > > ... is it something "obvious" like:
> > >
> > > diff --git a/include/linux/secretmem.h b/include/linux/secretmem.h
> > > index 35f3a4a8ceb1e..6996f1f53f147 100644
> > > --- a/include/linux/secretmem.h
> > > +++ b/include/linux/secretmem.h
> > > @@ -16,7 +16,7 @@ static inline bool folio_is_secretmem(struct folio *folio)
> > > * We know that secretmem pages are not compound and LRU so we can
> > > * save a couple of cycles here.
> > > */
> > > - if (folio_test_large(folio) || !folio_test_lru(folio))
> > > + if (folio_test_large(folio) || folio_test_lru(folio))
> > > return false;
> > > mapping = (struct address_space *)
> >
> > ... yes, that does the trick!
> >
>
> Proper patch (I might send out again on Monday "officially"). There are
> other improvements we want to do to folio_is_secretmem() in the light of
> folio_fast_pin_allowed(), that I wanted to do a while ago. I might send
> a patch for that as well now that I'm at it.
The most robust but a bit slower solution is to make folio_is_secretmem()
call folio_mapping() rather than open code the check.
What improvements did you have in mind?
> From 85558a46d9f249f26bd77dd3b18d14f248464845 Mon Sep 17 00:00:00 2001
> From: David Hildenbrand <david@xxxxxxxxxx>
> Date: Fri, 22 Mar 2024 22:45:36 +0100
> Subject: [PATCH] mm/secretmem: fix GUP-fast succeeding on secretmem folios
>
> folio_is_secretmem() states that secretmem folios cannot be LRU folios:
> so we may only exit early if we find an LRU folio. Yet, we exit early if
> we find a folio that is not a secretmem folio.
>
> Consequently, folio_is_secretmem() fails to detect secretmem folios and,
> therefore, we can succeed in grabbing a secretmem folio during GUP-fast,
> crashing the kernel when we later try reading/writing to the folio, because
> the folio has been unmapped from the directmap.
>
> Reported-by: xingwei lee <xrivendell7@xxxxxxxxx>
> Reported-by: yue sun <samsun1006219@xxxxxxxxx>
> Debugged-by: Miklos Szeredi <miklos@xxxxxxxxxx>
> Fixes: 1507f51255c9 ("mm: introduce memfd_secret system call to create "secret" memory areas")
> Cc: <stable@xxxxxxxxxxxxxxx>
> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
Reviewed-by: Mike Rapoport (IBM) <rppt@xxxxxxxxxx>
> ---
> include/linux/secretmem.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/secretmem.h b/include/linux/secretmem.h
> index 35f3a4a8ceb1..6996f1f53f14 100644
> --- a/include/linux/secretmem.h
> +++ b/include/linux/secretmem.h
> @@ -16,7 +16,7 @@ static inline bool folio_is_secretmem(struct folio *folio)
> * We know that secretmem pages are not compound and LRU so we can
> * save a couple of cycles here.
> */
> - if (folio_test_large(folio) || !folio_test_lru(folio))
> + if (folio_test_large(folio) || folio_test_lru(folio))
> return false;
> mapping = (struct address_space *)
> --
> 2.43.2
>
>
> --
> Cheers,
>
> David / dhildenb
>
--
Sincerely yours,
Mike.
