linux-kernel - Re: mremap vs sysctl_max_map

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: <a11a10b5-4a31-2537-7b14-83f4b22e5f6c@suse.cz>
Date:   Mon, 18 Feb 2019 10:57:18 +0100
From:   Vlastimil Babka <vbabka@...e.cz>
To:     Oscar Salvador <osalvador@...e.de>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, linux-api@...r.kernel.org
Cc:     hughd@...gle.com, viro@...iv.linux.org.uk,
        torvalds@...ux-foundation.org
Subject: Re: mremap vs sysctl_max_map_count

On 2/18/19 9:33 AM, Oscar Salvador wrote:
> 
> Hi all,
> 
> I would like to bring up a topic that comes from an issue a customer of ours
> is facing with the mremap syscall + hitting the max_map_count threshold:
> 
> When passing the MREMAP_FIXED flag, mremap() calls mremap_to() which does the
> following:
> 
> 1) it unmaps the region where we want to put the new map:
>    (new_addr, new_addr + new_len] [1]
> 2) IFF old_len > new_len, it unmaps the region:
>    (old_addr + new_len, (old_addr + new_len) + (old_len - new_len)] [2]
> 
> Now, having gone through steps 1) and 2), we eventually call move_vma() to do
> the actual move.
> 
> move_vma() checks if we are at least 4 maps below max_map_count, otherwise
> it bails out with -ENOMEM [3].
> The problem is that we might have already unmapped the vma's in steps 1) and 2),
> so it is not possible for userspace to figure out the state of the vma's after
> it gets -ENOMEM.
> 
> - Did new_addr got unmaped?
> - Did part of the old_addr got unmaped?
> 
> Because of that, it gets tricky for userspace to clean up properly on error
> path.
> 
> While it is true that we can return -ENOMEM for more reasons
> (e.g: see vma_to_resize()->may_expand_vm()), I think that we might be able to
> pre-compute the number of maps that we are going add/release during the first
> two do_munmaps(), and check whether we are 4 maps below the threshold
> (as move_vma() does).
> Should not be the case, we can bail out early before we unmap anything, so we
> make sure the vma's are left untouched in case we are going to be short of maps.
> 
> I am not sure if that is realistically doable, or there are limitations
> I overlooked, or we simply do not want to do that.

IMHO it makes sense to do all such resource limit checks upfront. It
should all be protected by mmap_sem and thus stable, right? Even if it
was racy, I'd think it's better to breach the limit a bit due to a race
than bail out in the middle of operation. Being also resilient against
"real" ENOMEM's due to e.g. failure to alocate a vma would be much
harder perhaps (but maybe it's already mostly covered by the
too-small-to-fail in page allocator), but I'd try with the artificial
limits at least.

> Before investing more time and giving it a shoot, I just wanted to bring
> this upstream to get feedback on this matter.
> 
> Thanks
> 
> [1] https://github.com/torvalds/linux/blob/master/mm/mremap.c#L519
> [2] https://github.com/torvalds/linux/blob/master/mm/mremap.c#L523
> [3] https://github.com/torvalds/linux/blob/master/mm/mremap.c#L338
>