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

[Dmitry Safonov]

On 01/12/2017 01:26 AM, Andy Lutomirski wrote:
> 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.

Indeed, forgot about the helper.

> Also, we need to verify that, if this is called execve(), it does the
> right thing.

Hmm, not sure I get it right.
A test for calling compat sys_execve() from and for 64-bit ELF?

> > +               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.

Well, yep, I also don't like how this code looks like.
That will need to add a parameter to find_start_end() whether
allocation is bottom-up or up-bottom.
I'll try to cleanup for v2.

> --Andy


--
             Dmitry