This is touching mm/mmap.c, please ensure to cc- the reviewers (me and
Liam, I have cc'd Liam here) as listed in MAINTAINERS when submitting
patches for this file.
Also this seems like a really speculative 'please discuss' change so should
be an RFC imo.
On Tue, Oct 08, 2024 at 12:55:39AM +0200, Jann Horn wrote:
> As explained in the comment block this change adds, we can't tell what
> userspace's intent is when the stack grows towards an inaccessible VMA.
>
> I have a (highly contrived) C testcase for 32-bit x86 userspace with glibc
> that mixes malloc(), pthread creation, and recursion in just the right way
> such that the main stack overflows into malloc() arena memory.
> (Let me know if you want me to share that.)
You are claiming a fixes from 2017 and cc'ing stable on a non-RFC so
yeah... we're going to need more than your word for it :) we will want to
be really sure this is a thing before we backport to basically every
stable kernel.
Surely this isn't that hard to demonstrate though? You mmap something
PROT_NONE just stack gap below the stack, then intentionally extend stack
to it, before mprotect()'ing the PROT_NONE region?
>
> I don't know of any specific scenario where this is actually exploitable,
> but it seems like it could be a security problem for sufficiently unlucky
> userspace.
>
> I believe we should ensure that, as long as code is compiled with something
> like -fstack-check, a stack overflow in it can never cause the main stack
> to overflow into adjacent heap memory.
>
> My fix effectively reverts the behavior for !vma_is_accessible() VMAs to
> the behavior before commit 1be7107fbe18 ("mm: larger stack guard gap,
> between vmas"), so I think it should be a fairly safe change even in
> case A.
>
> Fixes: 561b5e0709e4 ("mm/mmap.c: do not blow on PROT_NONE MAP_FIXED holes in the stack")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Jann Horn <jannh@xxxxxxxxxx>
> ---
> I have tested that Libreoffice still launches after this change, though
> I don't know if that means anything.
>
> Note that I haven't tested the growsup code.
> ---
> mm/mmap.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 46 insertions(+), 7 deletions(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index dd4b35a25aeb..971bfd6c1cea 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1064,10 +1064,12 @@ static int expand_upwards(struct vm_area_struct *vma, unsigned long address)
> gap_addr = TASK_SIZE;
>
> next = find_vma_intersection(mm, vma->vm_end, gap_addr);
> - if (next && vma_is_accessible(next)) {
> - if (!(next->vm_flags & VM_GROWSUP))
> + if (next && !(next->vm_flags & VM_GROWSUP)) {
> + /* see comments in expand_downwards() */
> + if (vma_is_accessible(prev))
> + return -ENOMEM;
> + if (address == next->vm_start)
> return -ENOMEM;
> - /* Check that both stack segments have the same anon_vma? */
I hate that we even maintain this for one single arch I believe at this point?
> }
>
> if (next)
> @@ -1155,10 +1157,47 @@ int expand_downwards(struct vm_area_struct *vma, unsigned long address)
> /* Enforce stack_guard_gap */
> prev = vma_prev(&vmi);
> /* Check that both stack segments have the same anon_vma? */
> - if (prev) {
> - if (!(prev->vm_flags & VM_GROWSDOWN) &&
> - vma_is_accessible(prev) &&
> - (address - prev->vm_end < stack_guard_gap))
> + if (prev && !(prev->vm_flags & VM_GROWSDOWN) &&
> + (address - prev->vm_end < stack_guard_gap)) {
> + /*
> + * If the previous VMA is accessible, this is the normal case
> + * where the main stack is growing down towards some unrelated
> + * VMA. Enforce the full stack guard gap.
> + */
> + if (vma_is_accessible(prev))
> + return -ENOMEM;
> +
> + /*
> + * If the previous VMA is not accessible, we have a problem:
> + * We can't tell what userspace's intent is.
> + *
> + * Case A:
> + * Maybe userspace wants to use the previous VMA as a
> + * "guard region" at the bottom of the main stack, in which case
> + * userspace wants us to grow the stack until it is adjacent to
> + * the guard region. Apparently some Java runtime environments
> + * and Rust do that?
> + * That is kind of ugly, and in that case userspace really ought
> + * to ensure that the stack is fully expanded immediately, but
> + * we have to handle this case.
Yeah we can't break userspace on this, no doubt somebody is relying on this
_somewhere_.
That said, I wish we disallowed this altogether regardless of accessibility.
> + *
> + * Case B:
> + * But maybe the previous VMA is entirely unrelated to the stack
> + * and is only *temporarily* PROT_NONE. For example, glibc
> + * malloc arenas create a big PROT_NONE region and then
> + * progressively mark parts of it as writable.
> + * In that case, we must not let the stack become adjacent to
> + * the previous VMA. Otherwise, after the region later becomes
> + * writable, a stack overflow will cause the stack to grow into
> + * the previous VMA, and we won't have any stack gap to protect
> + * against this.
Should be careful with terminology here, an mprotect() will not allow a
merge so by 'grow into' you mean that a stack VMA could become immediately
adjacent to a non-stack VMA prior to it which was later made writable.
Perhaps I am being pedantic...
> + *
> + * As an ugly tradeoff, enforce a single-page gap.
> + * A single page will hopefully be small enough to not be
> + * noticed in case A, while providing the same level of
> + * protection in case B that normal userspace threads get.
> + */
> + if (address == prev->vm_end)
> return -ENOMEM;
Ugh, yuck. Not a fan of this at all. Feels like a dreadful hack.
You do raise an important point here, but it strikes me that we should be
doing this check in the mprotect()/mmap() MAP_FIXED scenarios where it
shouldn't be too costly to check against the next VMA (which we will be
obtaining anyway for merge checks)?
That way we don't need a hack like this, and can just disallow the
operation. That'd probably be as liable to break the program as an -ENOMEM
on a stack expansion would...
> }
>
>
> ---
> base-commit: 8cf0b93919e13d1e8d4466eb4080a4c4d9d66d7b
> change-id: 20241008-stack-gap-inaccessible-c7319f7d4b1b
> --
> Jann Horn <jannh@xxxxxxxxxx>
>
