linux-kernel - Re: [PATCH hotfix 6.12] mm/mmap: correct error handling in mmap

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: <bae80e71-a139-4209-a2f8-3dd08d1b2c5b@lucifer.local>
Date: Tue, 1 Oct 2024 17:51:07 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Vlastimil Babka <vbabka@...e.cz>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, Bert Karwatzki <spasswolf@....de>,
        Vegard Nossum <vegard.nossum@...cle.com>
Subject: Re: [PATCH hotfix 6.12] mm/mmap: correct error handling in
 mmap_region()

On Tue, Oct 01, 2024 at 06:42:47PM GMT, Vlastimil Babka wrote:
> On 10/1/24 15:37, Lorenzo Stoakes wrote:
> > Commit f8d112a4e657 ("mm/mmap: avoid zeroing vma tree in mmap_region()")
> > changed how error handling is performed in mmap_region().
> >
> > The error value defaults to -ENOMEM, but then gets reassigned immediately
> > to the result of vms_gather_munmap_vmas() if we are performing a MAP_FIXED
> > mapping over existing VMAs (and thus unmapping them).
> >
> > This overwrites the error value, potentially clearing it.
> >
> > After this, we invoke may_expand_vm() and possibly vm_area_alloc(), and
> > check to see if they failed. If they do so, then we perform error-handling
> > logic, but importantly, we do NOT update the error code.
> >
> > This means that, if vms_gather_munmap_vmas() succeeds, but one of these
> > calls does not, the function will return indicating no error, but rather an
> > address value of zero, which is entirely incorrect.
> >
> > Correct this and avoid future confusion by strictly setting error on each
> > and every occasion we jump to the error handling logic, and set the error
> > code immediately prior to doing so.
> >
> > This way we can see at a glance that the error code is always correct.
> >
> > Many thanks to Vegard Nossum who spotted this issue in discussion around
> > this problem.
> >
> > Reported-by: Bert Karwatzki <spasswolf@....de>
> > Link: https://lore.kernel.org/all/20241001023402.3374-1-spasswolf@web.de/
>
> I'd be surprised if that fixed the reported issue, but yeah this is a bug to
> fix anyway.

Yeah it seems that you brought about a tear in the space-time continuum and
the very moment you said that Bert reported that yes sadly this doesn't fix
it :>)

I thought maybe some very weird bug by this function returning a non-error
result (of zero!) when it should have failed might have triggered some
later maple tree corruption but yeah, long shot I guess :(

Still, ultimately it spawned from the report and forms part of an overall
fix of the function so may as well vaguely keep the R-b tag, I kept it a
link rather than 'closes' so should be fine.

>
> > Suggested-by: Vegard Nossum <vegard.nossum@...cle.com>
> > Fixes: f8d112a4e657 ("mm/mmap: avoid zeroing vma tree in mmap_region()")
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
>
> Reviewed-by: Vlastimil Babka <vbabka@...e.cz>
>
>

Thanks!