Messages in this thread | | | From | Andy Lutomirski <> | Date | Wed, 11 Jan 2017 14:26:13 -0800 | Subject | Re: [PATCH 1/2] x86/mm: don't mmap() over 4GB with compat syscall |
| |
On Wed, Jan 11, 2017 at 10:17 AM, Dmitry Safonov <dsafonov@virtuozzo.com> wrote: > During fixing CRIU bugs on ZDTM tests for 32-bit C/R, I found that > compatible ia32/x32 syscalls mmap() and mmap2() can return address > over 4Gb in x86_64 applications, which results in returning lower > 4 bytes of address while dropping the higher bytes. > It happens because mmap() upper limit doesn't differ native/compat > syscalls for 64-bit task, it's: (TASK_UNMAPPED_BASE + random_factor) > which is: (PAGE_ALIGN(TASK_SIZE / 3)) + random_factor > (in case of legacy mmap it's just TASK_SIZE). > This patch limits higher address that can be mmaped with compat > syscalls in 64-bit applications with IA32_PAGE_OFFSET (+randomization). > > Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com> > --- > arch/x86/kernel/sys_x86_64.c | 37 +++++++++++++++++++++++++++++-------- > 1 file changed, 29 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/kernel/sys_x86_64.c b/arch/x86/kernel/sys_x86_64.c > index a55ed63b9f91..0893725db6e6 100644 > --- a/arch/x86/kernel/sys_x86_64.c > +++ b/arch/x86/kernel/sys_x86_64.c > @@ -100,7 +100,7 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len, > static void find_start_end(unsigned long flags, unsigned long *begin, > unsigned long *end) > { > - if (!test_thread_flag(TIF_ADDR32) && (flags & MAP_32BIT)) { > + if (!test_thread_flag(TIF_ADDR32)) { > /* This is usually used needed to map code in small > model, so it needs to be in the first 31bit. Limit > it to that. This means we need to move the > @@ -109,14 +109,24 @@ static void find_start_end(unsigned long flags, unsigned long *begin, > malloc knows how to fall back to mmap. Give it 1GB > of playground for now. -AK */ > *begin = 0x40000000; > - *end = 0x80000000; > - if (current->flags & PF_RANDOMIZE) { > - *begin = randomize_page(*begin, 0x02000000); > + > + if (flags & MAP_32BIT) { > + if (current->flags & PF_RANDOMIZE) > + *begin = randomize_page(*begin, 0x02000000); > + *end = 0x80000000; > + return; > + } > + if (current->thread.status & TS_COMPAT) { > + if (current->flags & PF_RANDOMIZE) > + *begin = randomize_page(*begin, > + 1UL << mmap_rnd_compat_bits); > + *end = IA32_PAGE_OFFSET; > + return; > } > - } else { > - *begin = current->mm->mmap_legacy_base; > - *end = TASK_SIZE; > } > + > + *begin = current->mm->mmap_legacy_base; > + *end = TASK_SIZE; > } > > unsigned long > @@ -187,10 +197,21 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0, > return addr; > } > > + if (current->thread.status & TS_COMPAT) {
in_compat_syscall(), please.
Also, we need to verify that, if this is called execve(), it does the right thing.
> + if (current->flags & PF_RANDOMIZE) { > + unsigned long rnd = 1UL << mmap_rnd_compat_bits; > + > + info.high_limit = > + randomize_page(IA32_PAGE_OFFSET - rnd, rnd); > + } else { > + info.high_limit = IA32_PAGE_OFFSET; > + } > + } else { > + info.high_limit = mm->mmap_base; > + }
This code was incomprehensible before and it's worse now. Could you try to clean it up a bit? For example, a patch that simply folds find_start_end() into its sole caller as the first patch in the series without changing any semantics would probably help.
--Andy
| |