| 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
| ||
|
Message-ID: <20171004150334.GB31992@x1>
Date: Wed, 4 Oct 2017 23:03:34 +0800
From: Baoquan He <bhe@...hat.com>
To: Michal Hocko <mhocko@...ne.org>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,
Kees Cook <keescook@...omium.org>,
Oleg Nesterov <oleg@...hat.com>, Jiri Kosina <jkosina@...e.cz>,
Al Viro <viro@...iv.linux.org.uk>, Ingo Molnar <mingo@...e.hu>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: MAP_FIXED for ELF mappings
On 10/04/17 at 09:50am, Michal Hocko wrote:
> Hi,
> while studying CVE-2017-1000253 and the MAP_FIXED usage in load_elf*
> code paths I have stumbled over MAP_FIXED usage for elf segments
> mapping. I am not really familiar with this area much so I might draw
> completely incorrect conclusions here but I am really wondering why we
> are doing MAP_FIXED there at all.
>
> I can see why some segments really have to be mapped at a specific
> address but I wonder whether MAP_FIXED is the right tool to achieve
> that. It seems to me that MAP_FIXED is fundamentally dangerous because
> it unmaps any existing mapping. I assume that nothing should be really
> mapped in the requested range that early so we can only stumble over
> something when the address space randomization place things unexpectedly
> (which was the case of the above mentioned CVE AFAIU).
>
> So my primary question is whether we can/should simply drop MAP_FIXED
> from elf_map at all. Instead we should test whether the mapping was
> successful for the requested address and fail otherwise. I realize that
> failing due to something that a user has no idea about sucks a lot but
> it seems to me safer to simply complain into the log and fail is a safer
> option.
Sorry to interrupt. I tried below example.c and example2.c files and
compile and link them with fpie and pie. Seems in the final PIE
executable, the local global is pc relative. Means the data segment has
to be put after the code segment though PIE program. Then MAP_FIXED is a
good to have flag, especially we have counted in the total_size to
search an area to cover the whole dynamic program and get the load_bias.
It's no way to fail to get map agrea in case load_addr_set == 1.
So with MAP_FIXED set, we won't take time to search the mm vma rb
tree when load_addr_set == 1, but just return the specified addr directly,
looks more efficient.
########### example2.c
int local_global_var=0x10;
int local_global_func(void)
{
return 0x40;
}
########## example.c
#include <stdio.h>
extern int local_global_var;
extern int local_global_func(void);
int main(void)
{
int x = local_global_func();
local_global_var= 0x20;
while(getchar()!= 'q')
continue;
return 0;
}
$gcc -fpie -c example.c example2.c
$ls example*.o
example.o example2.o
$objdump -d -r example.o
...
0000000000000000 <main>:
0: 55 push %rbp
1: 48 89 e5 mov %rsp,%rbp
4: 48 83 ec 10 sub $0x10,%rsp
8: e8 00 00 00 00 callq d <main+0xd>
9: R_X86_64_PLT32 local_global_func-0x4
d: 89 45 fc mov %eax,-0x4(%rbp)
10: c7 05 00 00 00 00 20 movl $0x20,0x0(%rip) # 1a
<main+0x1a>
17: 00 00 00
12: R_X86_64_PC32 local_global_var-0x8
1a: eb 01 jmp 1d <main+0x1d>
1c: 90 nop
1d: e8 00 00 00 00 callq 22 <main+0x22>
1e: R_X86_64_PLT32 getchar-0x4
22: 83 f8 71 cmp $0x71,%eax
25: 75 f5 jne 1c <main+0x1c>
27: b8 00 00 00 00 mov $0x0,%eax
2c: c9 leaveq
2d: c3 retq
...
$gcc -pie -o test example.o example2.o
$objdump -d -r test
...
0000000000000655 <main>:
655: 55 push %rbp
656: 48 89 e5 mov %rsp,%rbp
659: 48 83 ec 10 sub $0x10,%rsp
65d: e8 e8 ff ff ff callq 64a <local_global_func>
662: 89 45 fc mov %eax,-0x4(%rbp)
665: c7 05 b5 09 20 00 20 movl $0x20,0x2009b5(%rip)
#201024 <local_global_var>
66c: 00 00 00
66f: eb 01 jmp 672 <main+0x1d>
671: 90 nop
672: e8 a9 fe ff ff callq 520 <getchar@plt>
677: 83 f8 71 cmp $0x71,%eax
67a: 75 f5 jne 671 <main+0x1c>
67c: b8 00 00 00 00 mov $0x0,%eax
681: c9 leaveq
682: c3 retq
683: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
68a: 00 00 00
68d: 0f 1f 00 nopl (%rax)
...
>
> Something like the following completely untested diff. Or am I
> completely missing the point of the MAP_FIXED purpose?
> ---
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 6466153f2bf0..09456e2add18 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -341,6 +341,29 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
>
> #ifndef elf_map
>
> +static unsigned long elf_vm_mmap(struct file *filep, unsigned long addr,
> + unsigned long size, int prot, int type, unsigned long off)
> +{
> + unsigned long map_addr;
> +
> + /*
> + * If caller requests the mapping at a specific place, make sure we fail
> + * rather than potentially clobber an existing mapping which can have
> + * security consequences (e.g. smash over the stack area).
> + */
> + map_addr = vm_mmap(filep, addr, size, prot, type & ~MAP_FIXED, off);
> + if (BAD_ADDR(map_addr))
> + return map_addr;
> +
> + if ((type & MAP_FIXED) && map_addr != addr) {
> + pr_info("Uhuuh, elf segement at %p requested but the memory is mapped already\n",
> + (void*)addr);
> + return -EAGAIN;
> + }
> +
> + return map_addr;
> +}
> +
> static unsigned long elf_map(struct file *filep, unsigned long addr,
> struct elf_phdr *eppnt, int prot, int type,
> unsigned long total_size)
> @@ -366,11 +389,11 @@ static unsigned long elf_map(struct file *filep, unsigned long addr,
> */
> if (total_size) {
> total_size = ELF_PAGEALIGN(total_size);
> - map_addr = vm_mmap(filep, addr, total_size, prot, type, off);
> + map_addr = elf_vm_mmap(filep, addr, total_size, prot, type, off);
> if (!BAD_ADDR(map_addr))
> vm_munmap(map_addr+size, total_size-size);
> } else
> - map_addr = vm_mmap(filep, addr, size, prot, type, off);
> + map_addr = elf_vm_mmap(filep, addr, size, prot, type, off);
>
> return(map_addr);
> }
> @@ -1215,7 +1238,7 @@ static int load_elf_library(struct file *file)
> eppnt++;
>
> /* Now use mmap to map the library into memory. */
> - error = vm_mmap(file,
> + error = elf_vm_mmap(file,
> ELF_PAGESTART(eppnt->p_vaddr),
> (eppnt->p_filesz +
> ELF_PAGEOFFSET(eppnt->p_vaddr)),
> --
> Michal Hocko
> SUSE Labs
Powered by blists - more mailing lists