Re: [PATCH v3 04/11] arm64, mm: Move generic mmap layout functions to mm

From: Alex Ghiti <alex@ghiti.fr>
To: Kees Cook <keescook@chromium.org>
Cc: Albert Ou <aou@eecs.berkeley.edu>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Palmer Dabbelt <palmer@sifive.com>,
	Will Deacon <will.deacon@arm.com>,
	Russell King <linux@armlinux.org.uk>,
	Ralf Baechle <ralf@linux-mips.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Christoph Hellwig <hch@infradead.org>,
	Linux-MM <linux-mm@kvack.org>, Paul Burton <paul.burton@mips.com>,
	linux-riscv@lists.infradead.org,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	James Hogan <jhogan@kernel.org>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-mips@vger.kernel.org, Christoph Hellwig <hch@lst.de>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	Luis Chamberlain <mcgrof@kernel.org>
Subject: Re: [PATCH v3 04/11] arm64, mm: Move generic mmap layout functions to mm
Date: Sun, 28 Apr 2019 10:27:52 -0400	[thread overview]
Message-ID: <cf4073ed-098f-779e-32e1-f3273622b115@ghiti.fr> (raw)
In-Reply-To: <CAGXu5j+NkQ+nwRShuKeHMwuy6++3x0QMS9djE=wUzUUtAkVf3g@mail.gmail.com>

On 4/18/19 10:19 AM, Kees Cook wrote:
> On Thu, Apr 18, 2019 at 12:55 AM Alex Ghiti <alex@ghiti.fr> wrote:
>> Regarding the help text, I agree that it does not seem to be frequent to
>> place
>> comment above config like that, I'll let Christoph and you decide what's
>> best. And I'll
>> add the possibility for the arch to define its own STACK_RND_MASK.
> Yeah, I think it's very helpful to spell out the requirements for new
> architectures with these kinds of features in the help text (see
> SECCOMP_FILTER for example).
>
>>> I think CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT should select
>>> CONFIG_ARCH_HAS_ELF_RANDOMIZE. It would mean moving
>>
>> I don't think we should link those 2 features together: an architecture
>> may want
>> topdown mmap and don't care about randomization right ?
> Given that the mmap randomization and stack randomization are already
> coming along for the ride, it seems weird to make brk randomization an
> optional feature (especially since all the of the architectures you're
> converting include it). I'd also like these kinds of security features
> to be available by default. So, I think one patch to adjust the MIPS
> brk randomization entropy and then you can just include it in this
> move.
>
>> Actually, I had to add those ifdefs for mmap_rnd_compat_bits, not
>> is_compat_task.
> Oh! In that case, use CONFIG_HAVE_ARCH_MMAP_RND_BITS. :) Actually,
> what would be maybe cleaner would be to add mmap_rnd_bits_min/max
> consts set to 0 for the non-CONFIG_HAVE_ARCH_MMAP_RND_BITS case at the
> top of mm/mmap.c.
>
> I really like this clean-up! I think we can move x86 to it too without
> too much pain. :)
>
Hi,

Just a short note to indicate that while working on v4, I realized this 
series had a some issues:

- I broke the case ARCH_HAS_ELF_RANDOMIZE selected but not
   ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT (which can happen on arm 
without MMU for example)
- I use mmap_rnd_bits unconditionnally whereas it might not be defined 
(it works for all arches I moved though)

The only clean solution I found for the first problem is to propose a 
common implementation for arch_randomize_brk
and arch_mmap_rnd, which is another series on its own and another good 
cleanup since every architecture uses
the same functions (except ppc, but that can be workarounded easily).
Just like moving x86 deserves its own series since it adds up 8/9 commits.
I am on vacations for 2 weeks, so I won't have time to submit another 
patchset before, sorry about that.

Thanks,

Alex