linux-kernel - Re: [PATCH 0/2] mremap: Fix newaddr hint with MREMAP

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]

Message-ID: <04b1f6c2-32be-4bc0-af0a-919c9c1ee33f@lucifer.local>
Date: Fri, 6 Dec 2024 18:52:36 +0000
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Jann Horn <jannh@...gle.com>
Cc: Brian Geffon <bgeffon@...gle.com>,
        "Liam R. Howlett" <Liam.Howlett@...cle.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, Marco Vanotti <mvanotti@...gle.com>
Subject: Re: [PATCH 0/2] mremap: Fix newaddr hint with MREMAP_DONTUNMAP

On Fri, Dec 06, 2024 at 07:42:51PM +0100, Jann Horn wrote:
> +mmap maintainers (maybe mm/mremap.c should be added to the file
> pattern for "MEMORY MAPPING" in "MAINTAINERS"? I'm not sure)

Yeah I think it's actually right to group together _all_ VMA-related operations
under the VMA entry, because we have interaction between them all mprotect,
mlock, etc. etc. etc.

I will send a patch in a second for this, because we do keep getting bitten by
this.

>
> On Fri, Dec 6, 2024 at 4:20 PM Brian Geffon <bgeffon@...gle.com> wrote:
> > mmap(2) allows for a destination address to be specified without
> > MAP_FIXED and in this situation it's a hint to get_unmapped_area().
> > This address need not be page aligned because get_unmapped_area() will
> > align the hint.
> >
> > In the case of mremap(2) with MREMAP_DONTUNMAP it shares a code path
> > with MREMAP_FIXED in mremap_to(), which means this function can be
> > called in 3 different scenarios: MREMAP_FIXED only, MREMAP_DONTUNMAP
> > only, or MREMAP_FIXED | MREMAP_DONTUNMAP. In the second case when only
> > MREMAP_DONTUNMAP is specified we don't need to do alignment or size
> > checks on newaddr because they will be passed to get_unmapped_area() and
> > dealt with appropriately.
> >
> > This patch corrects that behavior to match what non-MREMAP_DONTUNMAP
> > mremap(2) and mmap(2) do. This odd behavioral difference was reported by
> > Marco Vanotti. Additionally, I've included a self test to validate this
> > behavior.

Yeah if this is user-facing - I don't think we can change this. Can we do any v2
as an RFC for now until we can get a chance to look at this? And please cc- the
VMA/mmap maintainers on future revisions (sorry this wasn't at all clear, we
need to update MAINTAINERS here).

Thanks!

>
> Marco pointed me to this; I had no idea mremap() had this undocumented
> behavior where it takes a hint address. The mremap() manpage is
> currently wrong about this, it sort of implies that the new_address
> argument is only used if MREMAP_FIXED is set.
>
> Marco also noticed that upstream glibc now assumes this behavior:
> https://sourceware.org/git/?p=glibc.git;a=commit;h=6c40cb0e9f893d49dc7caee580a055de53562206
>
> Debian also has a test that explicitly checks for this behavior:
> https://sources.debian.org/src/glibc/2.40-4/debian/patches/git-updates.diff/?hl=22820#L22818
>
> I guess it's too late to remove that behavior at this point, and the
> right thing to do is to update the manpage?

Yeah, if user-facing we can't fundamentally change behaviour even if it's
strange I'd say.