linux-kernel - Re: [PATCHv2] x86/vdso: add mremap hook to vm_special

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 14 Apr 2016 15:58:55 -0700
From:	Andy Lutomirski <luto@...capital.net>
To:	Dmitry Safonov <dsafonov@...tuozzo.com>
Cc:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>, X86 ML <x86@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	Dmitry Safonov <0x7f454c46@...il.com>
Subject: Re: [PATCHv2] x86/vdso: add mremap hook to vm_special_mapping

On Thu, Apr 14, 2016 at 9:32 AM, Dmitry Safonov <dsafonov@...tuozzo.com> wrote:
> Add possibility for userspace 32-bit applications to move
> vdso mapping. Previously, when userspace app called
> mremap for vdso, in return path it would land on previous
> address of vdso page, resulting in segmentation violation.
> Now it lands fine and returns to userspace with remapped vdso.
> This will also fix context.vdso pointer for 64-bit, which does not
> affect the user of vdso after mremap by now, but this may change.
>
> Renamed and moved text_mapping structure declaration inside
> map_vdso, as it used only there and now it complement
> vvar_mapping variable.
>
> There is still problem for remapping vdso in 32-bit glibc applications:
> linker relocates addresses for syscalls on vdso page, so
> you need to relink with the new addresses. Or the next syscall
> through glibc may fail:
>   Program received signal SIGSEGV, Segmentation fault.
>   #0  0xf7fd9b80 in __kernel_vsyscall ()
>   #1  0xf7ec8238 in _exit () from /usr/lib32/libc.so.6
>
> Signed-off-by: Dmitry Safonov <dsafonov@...tuozzo.com>
> ---
> v2: added __maybe_unused for pt_regs in vdso_mremap
>
>  arch/x86/entry/vdso/vma.c | 33 ++++++++++++++++++++++++++++-----
>  include/linux/mm_types.h  |  3 +++
>  mm/mmap.c                 | 10 ++++++++++
>  3 files changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/entry/vdso/vma.c b/arch/x86/entry/vdso/vma.c
> index 10f704584922..7e261e2554c8 100644
> --- a/arch/x86/entry/vdso/vma.c
> +++ b/arch/x86/entry/vdso/vma.c
> @@ -12,6 +12,7 @@
>  #include <linux/random.h>
>  #include <linux/elf.h>
>  #include <linux/cpu.h>
> +#include <linux/ptrace.h>
>  #include <asm/pvclock.h>
>  #include <asm/vgtod.h>
>  #include <asm/proto.h>
> @@ -98,10 +99,26 @@ static int vdso_fault(const struct vm_special_mapping *sm,
>         return 0;
>  }
>
> -static const struct vm_special_mapping text_mapping = {
> -       .name = "[vdso]",
> -       .fault = vdso_fault,
> -};
> +static int vdso_mremap(const struct vm_special_mapping *sm,
> +                     struct vm_area_struct *new_vma)
> +{
> +       struct pt_regs __maybe_unused *regs = current_pt_regs();
> +
> +#if defined(CONFIG_X86_32) || defined(CONFIG_IA32_EMULATION)
> +       /* Fixing userspace landing - look at do_fast_syscall_32 */
> +       if (regs->ip == (unsigned long)current->mm->context.vdso +
> +                       vdso_image_32.sym_int80_landing_pad
> +#ifdef CONFIG_IA32_EMULATION
> +               && current_thread_info()->status & TS_COMPAT
> +#endif

Instead of ifdef, use the (grossly misnamed) is_ia32_task() helper for
this, please.

> +          )
> +               regs->ip = new_vma->vm_start +
> +                       vdso_image_32.sym_int80_landing_pad;
> +#endif
> +       new_vma->vm_mm->context.vdso = (void __user *)new_vma->vm_start;

Can you arrange for the mremap call to fail if the old mapping gets
split?  This might be as simple as confirming that the new mapping's
length is what we expect it to be and, if it isn't, returning -EINVAL.

If anyone things that might break some existing application (which is
quite unlikely), then we could allow mremap to succeed but skip the
part where we change context.vdso and rip.

> +
> +       return 0;
> +}
>
>  static int vvar_fault(const struct vm_special_mapping *sm,
>                       struct vm_area_struct *vma, struct vm_fault *vmf)
> @@ -162,6 +179,12 @@ static int map_vdso(const struct vdso_image *image, bool calculate_addr)
>         struct vm_area_struct *vma;
>         unsigned long addr, text_start;
>         int ret = 0;
> +
> +       static const struct vm_special_mapping vdso_mapping = {
> +               .name = "[vdso]",
> +               .fault = vdso_fault,
> +               .mremap = vdso_mremap,
> +       };

Why did you add this instead of modifying text_mapping?

--Andy