Re: [PATCH 1/2] x86/mm: don't mmap() over 4GB with compat syscall

From: Andy Lutomirski
Date: Wed Jan 11 2017 - 17:26:39 EST


On Wed, Jan 11, 2017 at 10:17 AM, Dmitry Safonov <dsafonov@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> ---
> 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