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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 16 Jan 2024 14:25:35 -0800
From: Yang Shi <shy828301@...il.com>
To: Suren Baghdasaryan <surenb@...gle.com>
Cc: Jiri Slaby <jirislaby@...nel.org>, Rik van Riel <riel@...riel.com>, 
	Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org, 
	linux-kernel@...r.kernel.org, kernel-team@...com, 
	Matthew Wilcox <willy@...radead.org>, Christoph Lameter <cl@...ux.com>
Subject: Re: [PATCH v2] mm: align larger anonymous mappings on THP boundaries

On Tue, Jan 16, 2024 at 1:58 PM Suren Baghdasaryan <surenb@...gle.com> wrote:
>
> On Tue, Jan 16, 2024 at 12:56 PM Yang Shi <shy828301@...il.com> wrote:
> >
> > On Tue, Jan 16, 2024 at 11:16 AM Suren Baghdasaryan <surenb@...gle.com> wrote:
> > >
> > > On Tue, Jan 16, 2024 at 4:09 AM Jiri Slaby <jirislaby@...nel.org> wrote:
> > > >
> > > > On 16. 01. 24, 12:53, Jiri Slaby wrote:
> > > > > Hi,
> > > > >
> > > > > On 09. 08. 22, 20:24, Rik van Riel wrote:
> > > > >> Align larger anonymous memory mappings on THP boundaries by
> > > > >> going through thp_get_unmapped_area if THPs are enabled for
> > > > >> the current process.
> > > > >>
> > > > >> With this patch, larger anonymous mappings are now THP aligned.
> > > > >> When a malloc library allocates a 2MB or larger arena, that
> > > > >> arena can now be mapped with THPs right from the start, which
> > > > >> can result in better TLB hit rates and execution time.
> > > > >
> > > > > This appears to break 32bit processes on x86_64 (at least). In
> > > > > particular, 32bit kernel or firefox builds in our build system.
> > > > >
> > > > > Reverting this on top of 6.7 makes it work again.
> > > > >
> > > > > Downstream report:
> > > > >   https://bugzilla.suse.com/show_bug.cgi?id=1218841
> > > > >
> > > > > So running:
> > > > > pahole -J --btf_gen_floats -j --lang_exclude=rust
> > > > > --skip_encoding_btf_inconsistent_proto --btf_gen_optimized .tmp_vmlinux.btf
> > > > >
> > > > > crashes or errors out with some random errors:
> > > > > [182671] STRUCT idr's field 'idr_next' offset=128 bit_size=0 type=181346
> > > > > Error emitting field
> > > > >
> > > > > strace shows mmap() fails with ENOMEM right before the errors:
> > > > > 1223  mmap2(NULL, 5783552, PROT_READ|PROT_WRITE,
> > > > > MAP_PRIVATE|MAP_ANONYMOUS, -1, 0 <unfinished ...>
> > > > > ...
> > > > > 1223  <... mmap2 resumed>)              = -1 ENOMEM (Cannot allocate
> > > > > memory)
> > > > >
> > > > > Note the .tmp_vmlinux.btf above can be arbitrary, but likely large
> > > > > enough. For reference, one is available at:
> > > > > https://decibel.fi.muni.cz/~xslaby/n/btf
> > > > >
> > > > > Any ideas?
> > > >
> > > > This works around the problem, of course (but is a band-aid, not a fix):
> > > >
> > > > --- a/mm/mmap.c
> > > > +++ b/mm/mmap.c
> > > > @@ -1829,7 +1829,7 @@ get_unmapped_area(struct file *file, unsigned long
> > > > addr, unsigned long len,
> > > >                   */
> > > >                  pgoff = 0;
> > > >                  get_area = shmem_get_unmapped_area;
> > > > -       } else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) {
> > > > +       } else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) &&
> > > > !in_32bit_syscall()) {
> > > >                  /* Ensures that larger anonymous mappings are THP
> > > > aligned. */
> > > >                  get_area = thp_get_unmapped_area;
> > > >          }
> > > >
> > > >
> > > > thp_get_unmapped_area() does not take care of the legacy stuff...
> > >
> > > This change also affects the entropy of allocations. With this patch
> > > Android test [1] started failing and it requires only 8 bits of
> > > entropy. The feedback from our security team:
> > >
> > > 8 bits of entropy is already embarrassingly low, but was necessary for
> > > 32 bit ARM targets with low RAM at the time. It's definitely not
> > > acceptable for 64 bit targets.
> >
> > Thanks for the report. Is it 32 bit only or 64 bit is also impacted?
> > If I understand the code correctly, it expects the address allocated
> > by malloc() is kind of randomized, right?
>
> Yes, correct, the test expects a certain level of address randomization.
> The test failure was reported while running kernel_virt_x86_64 target
> (Android emulator on x86), so it does impact 64bit targets.

IIUC this breaks the "expectation" for randomized addresses returned
by malloc(), but it doesn't break any real Android application, right?
So this is a security concern instead of a real regression.

I think we can make this opt-in with a knob. Anyone who outweighs
security could opt this feature out. However I'm wondering whether
Android should implement a general address randomization mechanism
instead of depending on "luck" if you do care about it.

>
> >
> > >
> > > Could this change be either reverted or made optional (opt-in/opt-out)?
> > > Thanks,
> > > Suren.
> > >
> > > [1] https://cs.android.com/android/platform/superproject/main/+/main:cts/tests/aslr/src/AslrMallocTest.cpp;l=130
> > >
> > > >
> > > > regards,
> > > > --
> > > > js
> > > > suse labs
> > > >
> > > >

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ