linux-kernel - Re: [PATCH] mm: perform all memfd seal checks in a single place

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] [day] [month] [year] [list]

Message-ID: <Z1dtU3oxvkwQlHHo@google.com>
Date: Mon, 9 Dec 2024 14:21:07 -0800
From: Isaac Manjarres <isaacmanjarres@...gle.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
	"Liam R . Howlett" <Liam.Howlett@...cle.com>,
	Muchun Song <muchun.song@...ux.dev>,
	Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>,
	Hugh Dickins <hughd@...gle.com>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, Kalesh Singh <kaleshsingh@...gle.com>
Subject: Re: [PATCH] mm: perform all memfd seal checks in a single place

On Mon, Dec 09, 2024 at 11:13:14AM +0000, Lorenzo Stoakes wrote:
> On Fri, Dec 06, 2024 at 09:28:46PM +0000, Lorenzo Stoakes wrote:
> > We no longer actually need to perform these checks in the f_op->mmap() hook
> > any longer.
> >
> > We already moved the operation which clears VM_MAYWRITE on a read-only
> > mapping of a write-sealed memfd in order to work around the restrictions
> > imposed by commit 5de195060b2e ("mm: resolve faulty mmap_region() error
> > path behaviour").
> >
> > There is no reason for us not to simply go ahead and additionally check to
> > see if any pre-existing seals are in place here rather than defer this to
> > the f_op->mmap() hook.
> >
> > By doing this we remove more logic from shmem_mmap() which doesn't belong
> > there, as well as doing the same for hugetlbfs_file_mmap(). We also remove
> > dubious shared logic in mm.h which simply does not belong there either.
> >
> > It makes sense to do these checks at the earliest opportunity, we know
> > these are shmem (or hugetlbfs) mappings whose relevant VMA flags will not
> > change from the invoking do_mmap() so there is simply no need to wait.
> >
> > This also means the implementation of further memfd seal flags can be done
> > within mm/memfd.c and also have the opportunity to modify VMA flags as
> > necessary early in the mapping logic.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> ----8<----
> From 6cfef80e2ea5154302ba9b1925acd8e77ea6cd18 Mon Sep 17 00:00:00 2001
> From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> Date: Mon, 9 Dec 2024 11:04:08 +0000
> Subject: [PATCH] mm: fix typos in !memfd inline stub
> 
> I typo'd the declaration of memfd_check_seals_mmap() in the case where
> CONFIG_MEMFD_CREATE is not defined, resulting in build failures.
> 
> Fix this, and correct the misspelling of vm_flags which should be
> vm_flags_ptr at the same time.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> ---
>  include/linux/memfd.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/memfd.h b/include/linux/memfd.h
> index d53408b0bd31..246daadbfde8 100644
> --- a/include/linux/memfd.h
> +++ b/include/linux/memfd.h
> @@ -24,7 +24,8 @@ static inline struct folio *memfd_alloc_folio(struct file *memfd, pgoff_t idx)
>  {
>  	return ERR_PTR(-EINVAL);
>  }
> -int memfd_check_seals_mmap(struct file *file, unsigned long *vm_flags)
> +static inline int memfd_check_seals_mmap(struct file *file,
> +					 unsigned long *vm_flags_ptr)
>  {
>  	return 0;
>  }
> --
> 2.47.1

Thanks for sending this out so quickly! I think this came out nicely,
and makes the memfd sealing code easier to comprehend :).

I applied both the patch that moves the memfd seal checks to one place and
the fix up patch, and tested it out on my Pixel 6 device. The device
boots, and I do not see any errors related to memfd. Please feel free
to add my tested-by tag to the patch:

Tested-by: Isaac J. Manjarres <isaacmanjarres@...gle.com>

Thanks,
Isaac