On Tue, Mar 07, 2023 at 04:01:56PM +0300, Alexey Izbyshev wrote:

On 2023-01-19 19:03, Joey Gouly wrote:

quoted

diff --git a/mm/mmap.c b/mm/mmap.c
index 87d929316d57..99a4d9e2b0d8 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c

@@ -2665,6 +2665,16 @@ unsigned long mmap_region(struct file *file,

unsigned long addr,
 		vma_set_anonymous(vma);
 	}

+	if (map_deny_write_exec(vma, vma->vm_flags)) {
+		error = -EACCES;
+		if (file)
+			goto close_and_free_vma;
+		else if (vma->vm_file)
+			goto unmap_and_free_vma;
+		else
+			goto free_vma;
+	}
+

Why is the cleanup dispatch logic duplicated here, instead of simply doing
"goto close_and_free_vma" (where basically the same dispatch is done)?

Yes, though that's only possible after commit cc8d1b097de7 ("mmap: clean
up mmap_region() unrolling") in 6.3-rc1. It's worth adding a separate
patch to simplify this before final 6.3.

quoted

diff --git a/mm/mprotect.c b/mm/mprotect.c
index 908df12caa26..bc0587df042f 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c

@@ -762,6 +762,11 @@ static int do_mprotect_pkey(unsigned long start,

size_t len,
 			break;
 		}

+		if (map_deny_write_exec(vma, newflags)) {
+			error = -EACCES;
+			goto out;
+		}
+

Why does this check use "goto out", thereby skipping post-loop cleanup,
instead of "break" like all other checks? This looks like a bug to me.

Ah, good point, thanks. I think that's a left-over from my early attempt
at this series. The loop was changed in 5.19 with commit 4a18419f71cd
("mm/mprotect: use mmu_gather") but the patch not updated.

So yeah, it needs fixing. Joey, could you please send fixes for both
issues above?

Thanks.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel