linux-kernel - Re: [PATCH hotfix 6.12 v2 4/8] mm: resolve faulty mmap

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a1ffccb8-3f53-4486-a250-282ddc7054dd@lucifer.local>
Date: Mon, 28 Oct 2024 20:43:08 +0000
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: "Liam R. Howlett" <Liam.Howlett@...cle.com>,
        Mark Brown <broonie@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Peter Xu <peterx@...hat.com>, linux-arm-kernel@...ts.infradead.org,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>, Aishwarya TCV <Aishwarya.TCV@....com>
Subject: Re: [PATCH hotfix 6.12 v2 4/8] mm: resolve faulty mmap_region()
 error path behaviour

On Mon, Oct 28, 2024 at 10:22:32AM -1000, Linus Torvalds wrote:
> On Mon, 28 Oct 2024 at 10:18, Lorenzo Stoakes
> <lorenzo.stoakes@...cle.com> wrote:
> >
> > I'm genuinely not opposed to a horrible, awful:
> >
> > #ifdef CONFIG_ARM64
> >         if (file && file->f_ops == shmem_file_operations)
> >                 vm_flags |= VM_MTE_ALLOWED;
> > #endif
> >
> > Early in the operation prior to the arch_validate_flags() check.
>
> I would just put it inside the arm64 code itself.
>
> IOW, get rid of the VM_MTE_ALLOWED flag entirely, and just make the
> arm64 arch_validate_flags() code do something like
>
>         if (flags & VM_MTE) {
>                 if (file->f_ops != shmem_file_operations)
>                         return false;
>         }
>
> and be done with it.
>
> Considering that we only have that horrendous arch_validate_flags()
> for two architectures, and that they both just have magical special
> cases for MTE-like behavior, I do think that just making it be a hack
> inside those functions is the way to go.
>
>               Linus

Ah yeah makes sense.

FWIW I just made a fix -for now- which implements it in the hideous way,
shown below.

We can maybe take that as a fix-patch for now and I can look at replacing
this tomorrow with something as you suggest properly.

My only concern is that arm people might not be happy and we get some hold
up here...

Thanks, Lorenzo


----8<----
>From fb6c15c74ba0db57f18b08fc6d1e901676f25bf6 Mon Sep 17 00:00:00 2001
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Date: Mon, 28 Oct 2024 20:36:49 +0000
Subject: [PATCH] mm: account for MTE in arm64 on mmap_region() operation

Correctly account for MTE on mmap_region(). We need to check this ahead of
the operation, the shmem mmap hook was doing it, but this is at a point
where a failure would mean we'd have to tear down a partially installed
VMA.

Avoid all this by adding a function to specifically handle this case.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
---
 mm/mmap.c  | 20 ++++++++++++++++++++
 mm/shmem.c |  3 ---
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index 8462de1ee583..83afa1ebfd75 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1575,6 +1575,24 @@ static unsigned long __mmap_region(struct file *file, unsigned long addr,
 	return error;
 }

+/*
+ * We check VMA flag validity early in the mmap() process, however this can
+ * cause issues for arm64 when using MTE, which requires that it be used with
+ * shmem and in this instance and only then is VM_MTE_ALLOWED set permitting
+ * this operation.
+ *
+ * To avoid having to tear down a partially complete mapping we do this ahead of
+ * time.
+ */
+static vm_flags_t arch_adjust_flags(struct file *file, vm_flags_t vm_flags)
+{
+	if (!IS_ENABLED(CONFIG_ARM64))
+		return vm_flags;
+
+	if (shmem_file(file))
+		return vm_flags | VM_MTE_ALLOWED;
+}
+
 unsigned long mmap_region(struct file *file, unsigned long addr,
 			  unsigned long len, vm_flags_t vm_flags, unsigned long pgoff,
 			  struct list_head *uf)
@@ -1586,6 +1604,8 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
 	if (map_deny_write_exec(vm_flags, vm_flags))
 		return -EACCES;

+	vm_flags = arch_adjust_flags(file, vm_flags);
+
 	/* Allow architectures to sanity-check the vm_flags. */
 	if (!arch_validate_flags(vm_flags))
 		return -EINVAL;
diff --git a/mm/shmem.c b/mm/shmem.c
index 4ba1d00fabda..e87f5d6799a7 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2733,9 +2733,6 @@ static int shmem_mmap(struct file *file, struct vm_area_struct *vma)
 	if (ret)
 		return ret;

-	/* arm64 - allow memory tagging on RAM-based files */
-	vm_flags_set(vma, VM_MTE_ALLOWED);
-
 	file_accessed(file);
 	/* This is anonymous shared memory if it is unlinked at the time of mmap */
 	if (inode->i_nlink)
--