Re: [PATCH v2 1/2] mm: Implement memory-deny-write-execute as a prctl

From: Catalin Marinas <catalin.marinas@arm.com>
To: Alexey Izbyshev <izbyshev@ispras.ru>
Cc: "Joey Gouly" <joey.gouly@arm.com>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	"Lennart Poettering" <lennart@poettering.net>,
	"Zbigniew Jędrzejewski-Szmek" <zbyszek@in.waw.pl>,
	"Alexander Viro" <viro@zeniv.linux.org.uk>,
	"Kees Cook" <keescook@chromium.org>,
	"Szabolcs Nagy" <szabolcs.nagy@arm.com>,
	"Mark Brown" <broonie@kernel.org>,
	"Jeremy Linton" <jeremy.linton@arm.com>,
	"Topi Miettinen" <toiwoton@gmail.com>,
	linux-mm@kvack.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	linux-abi-devel@lists.sourceforge.net, nd@arm.com,
	shuah@kernel.org
Subject: Re: [PATCH v2 1/2] mm: Implement memory-deny-write-execute as a prctl
Date: Wed, 8 Mar 2023 12:36:59 +0000	[thread overview]
Message-ID: <ZAiBazZQ0Yrwqpqw@arm.com> (raw)
In-Reply-To: <8408d8901e9d7ee6b78db4c6cba04b78@ispras.ru>

On Tue, Mar 07, 2023 at 04:01:56PM +0300, Alexey Izbyshev wrote:
> On 2023-01-19 19:03, Joey Gouly wrote:
> > 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.

> > 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