Re: [tip:x86/urgent] x86/mpx: Fix recursive munmap() corruption

Re: [tip:x86/urgent] x86/mpx: Fix recursive munmap() corruption

[Sasha Levin]

Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 1de4fa14ee25 x86, mpx: Cleanup unused bound tables.

The bot has tested the following trees: v5.0.8, v4.19.35, v4.14.112, v4.9.169, v4.4.178.

v5.0.8: Build OK!
v4.19.35: Failed to apply! Possible dependencies:
    dd2283f2605e ("mm: mmap: zap pages with read mmap_sem in munmap")

v4.14.112: Failed to apply! Possible dependencies:
    dd2283f2605e ("mm: mmap: zap pages with read mmap_sem in munmap")

v4.9.169: Failed to apply! Possible dependencies:
    010426079ec1 ("sched/headers: Prepare for new header dependencies before moving more code to <linux/sched/mm.h>")
    1b028f784e8c ("x86/mm: Introduce mmap_compat_base() for 32-bit mmap()")
    39bc88e5e38e ("arm64: Disable TTBR0_EL1 during normal kernel execution")
    3f07c0144132 ("sched/headers: Prepare for new header dependencies before moving code to <linux/sched/signal.h>")
    44b04912fa72 ("x86/mpx: Do not allow MPX if we have mappings above 47-bit")
    6a0b41d1e23d ("x86/mm: Introduce arch_rnd() to compute 32/64 mmap random base")
    7c0f6ba682b9 ("Replace <asm/uaccess.h> with <linux/uaccess.h> globally")
    8f3e474f3cea ("x86/mm: Add task_size parameter to mmap_base()")
    9cf09d68b89a ("arm64: xen: Enable user access before a privcmd hvc call")
    bd38967d406f ("arm64: Factor out PAN enabling/disabling into separate uaccess_* macros")
    e13b73dd9c80 ("x86/hugetlb: Adjust to the new native/compat mmap bases")

v4.4.178: Failed to apply! Possible dependencies:
    1b028f784e8c ("x86/mm: Introduce mmap_compat_base() for 32-bit mmap()")
    2b5e869ecfcb ("MIPS: ELF: Interpret the NAN2008 file header flag")
    2ed02dd415ae ("MIPS: Use a union to access the ELF file header")
    44b04912fa72 ("x86/mpx: Do not allow MPX if we have mappings above 47-bit")
    5fa393c85719 ("MIPS: Break down cacheops.h definitions")
    694977006a7b ("MIPS: Use enums to make asm/pgtable-bits.h readable")
    745f35587846 ("MIPS: mm: Unify pte_page definition")
    780602d740fc ("MIPS: mm: Standardise on _PAGE_NO_READ, drop _PAGE_READ")
    7939469da29a ("MIPS64: signal: Fix o32 sigaction syscall")
    7b2cb64f91f2 ("MIPS: mm: Fix MIPS32 36b physical addressing (alchemy, netlogic)")
    8f3e474f3cea ("x86/mm: Add task_size parameter to mmap_base()")
    97f2645f358b ("tree-wide: replace config_enabled() with IS_ENABLED()")
    9e08f57d684a ("x86: mm: support ARCH_MMAP_RND_BITS")
    a60ae81e5e59 ("MIPS: CM: Fix mips_cm_max_vp_width for UP kernels")
    b1b4fad5cc67 ("MIPS: seccomp: Support compat with both O32 and N32")
    b27873702b06 ("mips, thp: remove infrastructure for handling splitting PMDs")
    b2edcfc81401 ("MIPS: Loongson: Add Loongson-3A R2 basic support")
    d07e22597d1d ("mm: mmap: add new /proc tunable for mmap_base ASLR")
    e13b73dd9c80 ("x86/hugetlb: Adjust to the new native/compat mmap bases")


How should we proceed with this patch?

--
Thanks,
Sasha

Re: [tip:x86/urgent] x86/mpx: Fix recursive munmap() corruption

[Dave Hansen]

On 4/18/19 11:29 AM, Sasha Levin wrote:
> This commit has been processed because it contains a "Fixes:" tag,
> fixing commit: 1de4fa14ee25 x86, mpx: Cleanup unused bound tables.
> 
> The bot has tested the following trees: v5.0.8, v4.19.35, v4.14.112, v4.9.169, v4.4.178.
> 
> v5.0.8: Build OK!
> v4.19.35: Failed to apply! Possible dependencies:
>     dd2283f2605e ("mm: mmap: zap pages with read mmap_sem in munmap")

I probably should have looked more closely at the state of the code
before dd2283f2605e.  A more correct Fixes: would probably have referred
to dd2283f2605e.  *It* appears to be the root cause rather than the
original MPX code that I called out.

The pre-dd2283f2605e code does this:

>         /*
>          * Remove the vma's, and unmap the actual pages
>          */
>         detach_vmas_to_be_unmapped(mm, vma, prev, end);
>         unmap_region(mm, vma, prev, start, end);
> 
>         arch_unmap(mm, vma, start, end);
> 
>         /* Fix up all other VM information */
>         remove_vma_list(mm, vma);

But, this is actually safe.  arch_unmap() can't see 'vma' in the rbtree
because it's been detached, so it can't do anything to 'vma' that might
be unsafe for remove_vma_list()'s use of 'vma'.	

The bug in dd2283f2605e was moving unmap_region() to the after arch_unmap().

I confirmed this by running the reproducer on v4.19.35.  It did not
trigger anything there, even with a bunch of debugging enabled which
detected the issue in 5.0.

Re: [tip:x86/urgent] x86/mpx: Fix recursive munmap() corruption

[Thomas Gleixner]

On Thu, 18 Apr 2019, Dave Hansen wrote:

> On 4/18/19 11:29 AM, Sasha Levin wrote:
> > This commit has been processed because it contains a "Fixes:" tag,
> > fixing commit: 1de4fa14ee25 x86, mpx: Cleanup unused bound tables.
> > 
> > The bot has tested the following trees: v5.0.8, v4.19.35, v4.14.112, v4.9.169, v4.4.178.
> > 
> > v5.0.8: Build OK!
> > v4.19.35: Failed to apply! Possible dependencies:
> >     dd2283f2605e ("mm: mmap: zap pages with read mmap_sem in munmap")
> 
> I probably should have looked more closely at the state of the code
> before dd2283f2605e.  A more correct Fixes: would probably have referred
> to dd2283f2605e.  *It* appears to be the root cause rather than the
> original MPX code that I called out.
> 
> The pre-dd2283f2605e code does this:
> 
> >         /*
> >          * Remove the vma's, and unmap the actual pages
> >          */
> >         detach_vmas_to_be_unmapped(mm, vma, prev, end);
> >         unmap_region(mm, vma, prev, start, end);
> > 
> >         arch_unmap(mm, vma, start, end);
> > 
> >         /* Fix up all other VM information */
> >         remove_vma_list(mm, vma);
> 
> But, this is actually safe.  arch_unmap() can't see 'vma' in the rbtree
> because it's been detached, so it can't do anything to 'vma' that might
> be unsafe for remove_vma_list()'s use of 'vma'.	
> 
> The bug in dd2283f2605e was moving unmap_region() to the after arch_unmap().
> 
> I confirmed this by running the reproducer on v4.19.35.  It did not
> trigger anything there, even with a bunch of debugging enabled which
> detected the issue in 5.0.

I'l amend the commit to avoid further confusion.

Thanks,

	tglx