Re: [PATCH v2] mm: mmap: allow for the maximum number of bits for randomizing mmap_base by default

[Rafael Aquini]

On Tue, Jun 11, 2024 at 02:32:39PM -0700, Andrew Morton wrote:
> On Mon, 10 Jun 2024 14:45:28 -0400 Rafael Aquini <aquini@xxxxxxxxxx> wrote:
> 
> > On Mon, Jun 10, 2024 at 11:11:39AM -0700, Andrew Morton wrote:
> > > On Thu,  6 Jun 2024 14:06:22 -0400 Rafael Aquini <aquini@xxxxxxxxxx> wrote:
> > > 
> > > > An ASLR regression was noticed [1] and tracked down to file-mapped areas
> > > > being backed by THP in recent kernels. The 21-bit alignment constraint
> > > > for such mappings reduces the entropy for randomizing the placement of
> > > > 64-bit library mappings and breaks ASLR completely for 32-bit libraries.
> > > > 
> > > > The reported issue is easily addressed by increasing vm.mmap_rnd_bits
> > > > and vm.mmap_rnd_compat_bits. This patch just provides a simple way to
> > > > set ARCH_MMAP_RND_BITS and ARCH_MMAP_RND_COMPAT_BITS to their maximum
> > > > values allowed by the architecture at build time.
> > > > 
> > > > [1] https://zolutal.github.io/aslrnt/
> > > 
> > > Are we able to identify a Fixes: target for this?
> > >
> > 
> > Sure, it would be:
> > 
> >  Fixes: 1854bc6e2420 ("mm/readahead: Align file mappings for non-DAX")
> >  
> > > I assume a cc:stable is appropriate?
> > > 
> > 
> > Andrew, I admit I was somewhat hesitant on adding the Fixes: and the stable CC
> > to this patch because I didn't really think of it as a "fix" for the given 
> > commit, but just as a simple way to toggle ARCH_MMAP_RND{,_COMPAT}_BITS 
> > to maximum allowed at build time.
> > 
> > I don't disagree with doing it, though, if you think it might be appropriate.
> 
> Well, "breaks completely" is motivational!
> 
> But does the patch fix this, by default?  Doesn't the user have to take
> some action (set FORCE_MAX_MMAP_RND_BITS) to fix the breakage?

Correct. The patch doesn't fix it by default but provides a way for users
to adjust these settings at buildtime. Users are still expected/required to 
take action, though.

> Shouldn't we make this the default (at least for 32-bit) so the
> regressed kernels are fixed simply by applying this patch?
> 

That is a fair take, indeed. I guess we could do something like

 config FORCE_MAX_MMAP_RND_BITS
        bool "Force maximum number of bits to use for ASLR of mmap base address"
-       default n
+       default y if !64BIT


> > Lemme know if you want me refreshing the patch to amend these bits.
> 
> Is OK, I can update things.

Thank you!

-- Rafael