login
Search
Hello,
I sent this patch 5 days ago, nobody replied. So I am giving it second
attempt.
Andrew, is it possible to test this in -mm branch?
Original mail follows:
Hi,
this is something like reaction to this thread:
http://lkml.org/lkml/2007/1/6/124. I hope I was able to separate the PIE
randomization part correctly.
There is platform specific (__i386__ only) part in exec shield, I am not
pretty sure why is it there, but wasn't brave enough to touch it. Can
someone comment the #ifdef out and test it on some other platform?
It will be nice to follow with brk randomization, but in exec-shield it is
afaik i386 only. Right?
Randomizes -pie compiled binaries. The implementation is part of Redhat's
exec-shield (http://people.redhat.com/mingo/exec-shield/).
Signed-off-by: Jan Kratochvil <honza@jikos.cz>
---
fs/binfmt_elf.c | 96 ++++++++++++++++++++++++++++++++++++++++++------------
1 files changed, 74 insertions(+), 22 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 9cc4f0a..1156f41 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -45,7 +45,7 @@
static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs);
static int load_elf_library(struct file *);
-static unsigned long elf_map (struct file *, unsigned long, struct elf_phdr *, int, int);
+static unsigned long elf_map (struct file *, unsigned long, struct elf_phdr *, int, int, unsigned long);
/*
* If we don't support core dumping, then supply a NULL so we
@@ -80,7 +80,7 @@ static struct linux_binfmt elf_format =
.hasvdso = 1
};
-#define BAD_ADDR(x) ((unsigned long)(x) >= TASK_SIZE)
+#define BAD_ADDR(x) ((unsigned long)(x) >= PAGE_MASK)
static int set_brk(unsigned long start, unsigned long end)
{
@@ -285,33 +285,70 @@ create_elf_tables(struct linux_binprm *b
#ifndef elf_map
static unsigned long elf_map(struct file *filep, unsigned long addr,
- struct elf_phdr *eppnt, int prot, int type)
+ struct elf_phdr *eppnt, ...On Fri, 11 May 2007 14:33:46 +0200 (CEST) I don't know what to do with this. The changelog doesn't tell me what PIE randomization _is_, nor why the kernel would want to do it. Your email client is space-stuffing the patches. That's easy to fix at the receiving end, but it'd be better to fix it at the sending end, please. -
I think it's precisely what we want to do in case the randomize_va_space is set to 1, don't we? (I haven't yet gone throught the patch though, so I am not sure whether this is the case). We already have stack randomization and mmap() base randomization but executable base randomization (which is of course only feasible for -pie executables) and brk() randomization still seem to be missing to make it complete. -- Jiri Kosina SUSE Labs -
On Fri, 11 May 2007 22:18:16 +0200 (CEST) erm, I was being funny. If you randomize a binary it won't run any more. cp /dev/random /bin/login. Oh well. My point is, we're not being told what is being randomized here. Is it the virtual starting address of the main executable mmap? Of the shared libraries also? Is it the stack location? What? I could reverse-engineer that info from the patch, I guess, but I'd prefer to go in the opposite direction: you tell us what the patch is trying to -
PIE = Position Independent Executable, that's how I named them. These are not regular executables, they are basically DSOs but usually compiled with -fpie/-fPIE instead of -fpic/-fPIC and linked with -pie instead of -shared to allow the compiled and linker perform more optimizations. See section 5 in http://people.redhat.com/drepper/nonselsec.pdf Jan unfortunately Ingo's document which doesn't really explain it. -
I've just quickly looked at the patch and it seems fine - it's using mmap()'s randomization functionality in such a way that it maps the the main executable of (specially compiled/linked) ET_DYN binaries onto a random address (in cases in which mmap() is allowed to perform a randomization). Which is what we want, I'd guess. Jan, would you care to update the patch with proper Changelog entry? However, I seem to get "soft" hang on boot with this patch, approximately at the time the init should be executed. The system is not completely stuck - interrupts are delivered, keyboard is working, alt-sysrq-t dumps proper output, but userspace doesn't seem to get started. This happens on i386, didn't try on other archs. -- Jiri Kosina -
Hi Jan,
I finally had time to look at it a little bit - I think you omitted
porting of proper handling of *interp_load_addr == 0, which made my box
hang. The patch below, when applied on top of what you have sent, makes it
work again and also the randomization for ET_DYN executables seems to work
OK.
Could you please refresh your patch, update the Changelog in a proper way
and resubmit?
Thanks.
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index be6671e..8406f9a 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -347,7 +347,7 @@ static inline unsigned long total_mappin
an ELF header */
static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
- struct file *interpreter, unsigned long *interp_load_addr,
+ struct file *interpreter, unsigned long *interp_map_addr,
unsigned long no_base)
{
struct elf_phdr *elf_phdata;
@@ -421,6 +421,9 @@ static unsigned long load_elf_interp(str
map_addr = elf_map(interpreter, load_addr + vaddr,
eppnt, elf_prot, elf_type, total_size);
+ total_size = 0;
+ if (!*interp_map_addr)
+ *interp_map_addr = map_addr;
error = map_addr;
if (BAD_ADDR(map_addr))
goto out_close;
-
Hi, sorry for insufficient description in my earlier post. I hope it is better this time. Jiri: Thanks for help, I applied your change on my previous patch. This patch is using mmap()'s randomization functionality in such a way that it maps the main executable of (specially compiled/linked -pie/-fpie) ET_DYN binaries onto a random address (in cases in which mmap() is allowed to perform a randomization). Origin of this patch is in exec-shield (http://people.redhat.com/mingo/exec-shield/) Signed-off-by: Jan Kratochvil <honza@jikos.cz> --- fs/binfmt_elf.c | 99 ++++++++++++++++++++++++++++++++++++++++++------------ 1 files changed, 77 insertions(+), 22 deletions(-) diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index fa8ea33..8406f9a 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -45,7 +45,7 @@ static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs); static int load_elf_library(struct file *); -static unsigned long elf_map (struct file *, unsigned long, struct elf_phdr *, int, int); +static unsigned long elf_map (struct file *, unsigned long, struct elf_phdr *, int, int, unsigned long); /* * If we don't support core dumping, then supply a NULL so we @@ -80,7 +80,7 @@ static struct linux_binfmt elf_format = .hasvdso = 1 }; -#define BAD_ADDR(x) ((unsigned long)(x) >= TASK_SIZE) +#define BAD_ADDR(x) ((unsigned long)(x) >= PAGE_MASK) static int set_brk(unsigned long start, unsigned long end) { @@ -285,33 +285,70 @@ create_elf_tables(struct linux_binprm *b #ifndef elf_map static unsigned long elf_map(struct file *filep, unsigned long addr, - struct elf_phdr *eppnt, int prot, int type) + struct elf_phdr *eppnt, int prot, int type, + unsigned long total_size) { unsigned long map_addr; - unsigned long pageoffset = ELF_PAGEOFFSET(eppnt->p_vaddr); + unsigned long size = eppnt->p_filesz + ELF_PAGEOFFSET(eppnt->p_vaddr); + unsigned long off = eppnt->p_offset - ...
Andrew, if you are going to merge this, you could add Signed-off-by: Jiri Kosina <jkosina@suse.cz> to this patch, if it makes any difference. Ingo, do you have any opinion on merging this upstream please? Thanks, -- Jiri Kosina -
On Thu, 17 May 2007 22:24:11 +0200 (CEST)
From: Andrew Morton <akpm@linux-foundation.org>
- the compiler knows how to inline things
- return -EINVAL on zero-size, not -EIO
- reduce scope of local `interp_map_addr', remove unneeded initialisation,
add needed comment.
- coding-style repairs
Cc: Jan Kratochvil <honza@jikos.cz>
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Roland McGrath <roland@redhat.com>
Cc: Jakub Jelinek <jakub@redhat.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
fs/binfmt_elf.c | 26 +++++++++++++++++---------
1 files changed, 17 insertions(+), 9 deletions(-)
diff -puN fs/binfmt_elf.c~pie-randomization-fix fs/binfmt_elf.c
--- a/fs/binfmt_elf.c~pie-randomization-fix
+++ a/fs/binfmt_elf.c
@@ -322,17 +322,17 @@ static unsigned long elf_map(struct file
#endif /* !elf_map */
-static inline unsigned long total_mapping_size(struct elf_phdr *cmds, int nr)
+static unsigned long total_mapping_size(struct elf_phdr *cmds, int nr)
{
int i, first_idx = -1, last_idx = -1;
- for (i = 0; i < nr; i++)
+ for (i = 0; i < nr; i++) {
if (cmds[i].p_type == PT_LOAD) {
last_idx = i;
if (first_idx == -1)
first_idx = i;
}
-
+ }
if (first_idx == -1)
return 0;
@@ -396,8 +396,10 @@ static unsigned long load_elf_interp(str
}
total_size = total_mapping_size(elf_phdata, interp_elf_ex->e_phnum);
- if (!total_size)
+ if (!total_size) {
+ error = -EINVAL;
goto out_close;
+ }
eppnt = elf_phdata;
for (i = 0; i < interp_elf_ex->e_phnum; i++, eppnt++) {
@@ -586,7 +588,8 @@ static int load_elf_binary(struct linux_
int elf_exec_fileno;
int retval, i;
unsigned int size;
- unsigned long elf_entry, interp_load_addr = 0, interp_map_addr = 0;
+ unsigned long elf_entry;
+ unsigned long interp_load_addr = 0;
unsigned long start_code, end_code, start_data, end_data;
unsigned long reloc_func_desc = 0;
char passed_fileno[6];
@@ -908,7 +911,7 @@ static ...I haven't reviewed this patch, but I have given it the same testing as Marcus' earlier PIE randomization patch, which had been heading into 2.6.20 until I reported strange build failures from it on i386, which went away when it got reverted. This PIE randomization patch from Jan gives me no problems (beyond s/^ / / to get it to apply). -
On Thu, 17 May 2007 22:24:11 +0200 (CEST) ia64: arch/ia64/ia32/binfmt_elf32.c:265: error: conflicting types for 'elf32_map' arch/ia64/ia32/../../../fs/binfmt_elf.c:48: error: previous declaration of 'elf32_map' was here arch/ia64/ia32/binfmt_elf32.c:265: error: conflicting types for 'elf32_map' arch/ia64/ia32/../../../fs/binfmt_elf.c:48: error: previous declaration of 'elf32_map' was here arch/ia64/ia32/../../../fs/binfmt_elf.c:48: warning: 'elf32_map' declared `static' but never defined arch/ia64/ia32/binfmt_elf32.c:265: warning: 'elf32_map' defined but not used -
The fix to this is trivial - just fix the prototype in ia32 compat version of ia64 code. Updated patch is below. From: Jan Kratochvil <honza@jikos.cz> This patch is using mmap()'s randomization functionality in such a way that it maps the main executable of (specially compiled/linked -pie/-fpie) ET_DYN binaries onto a random address (in cases in which mmap() is allowed to perform a randomization). Origin of this patch is in exec-shield (http://people.redhat.com/mingo/exec-shield/) Signed-off-by: Jan Kratochvil <honza@jikos.cz> Signed-off-by: Jiri Kosina <jkosina@suse.cz> Cc: Ingo Molnar <mingo@elte.hu> Cc: Roland McGrath <roland@redhat.com> Cc: Jakub Jelinek <jakub@redhat.com> --- arch/ia64/ia32/binfmt_elf32.c | 2 +- fs/binfmt_elf.c | 109 ++++++++++++++++++++++++++++++++--------- 2 files changed, 87 insertions(+), 24 deletions(-) diff --git a/arch/ia64/ia32/binfmt_elf32.c b/arch/ia64/ia32/binfmt_elf32.c index c05bda6..6f4d3d0 100644 --- a/arch/ia64/ia32/binfmt_elf32.c +++ b/arch/ia64/ia32/binfmt_elf32.c @@ -261,7 +261,7 @@ elf32_set_personality (void) } static unsigned long -elf32_map (struct file *filep, unsigned long addr, struct elf_phdr *eppnt, int prot, int type) +elf32_map (struct file *filep, unsigned long addr, struct elf_phdr *eppnt, int prot, int type, unsigned long unused) { unsigned long pgoff = (eppnt->p_vaddr) & ~IA32_PAGE_MASK; diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index fa8ea33..abcac6a 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -45,7 +45,7 @@ static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs); static int load_elf_library(struct file *); -static unsigned long elf_map (struct file *, unsigned long, struct elf_phdr *, int, int); +static unsigned long elf_map (struct file *, unsigned long, struct elf_phdr *, int, int, unsigned long); /* * If we don't support core dumping, then supply a NULL so we @@ -80,7 +80,7 @@ static struct linux_binfmt ...
The above highlighted changes are the cause of random segfaults of PIE binaries. See https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=246623 The problem is if ld.so is prelinked to some address in the area where the kernel actually maps it, particularly if elf_map in load_elf_interp returns an address one page below its first PT_LOAD segments vaddr. Then load_addr (it is a load bias actually) returned from load_elf_interp is 0xfffff000 (on 32-bit kernels) and BAD_ADDR are all addresses >= 0xfffff000 (on i?86). The fix should be either changing the definition of BAD_ADDR to e.g. IS_ERR_VALUE(x), or at least changing the if (!BAD_ADDR(elf_entry)) { above to if (!IS_ERR_VALUE(elf_entry)) {, the second BAD_ADDR can already stay, because at that place elf_entry is no longer a bias (difference between actual and preferred load address), but an actual address, where very high addresses are of course invalid. Jakub -
Thanks a lot for pointing this out. Andrew, could this be folded into pie-randomization.patch please? From: Jiri Kosina <jkosina@suse.cz> pie randomization: fix BAD_ADDR macro pie-randomization.patch makes the load_addr in load_elf_interp() the load bias of ld.so (difference between the actual load base address and first PT_LOAD segment's p_vaddr). If the difference equals (on x86) to 0xfffff000 (which is valid [1]), SIGSEGV is incorrectly sent. This patch changes the BAD_ADDR so that it catches the mappings to the error-area properly. [1] https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=246623 Reported-by: Jakub Jelinek <jakub@redhat.com> Signed-off-by: Jiri Kosina <jkosina@suse.cz> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index da270d1..466477d 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -80,7 +80,7 @@ static struct linux_binfmt elf_format = { .hasvdso = 1 }; -#define BAD_ADDR(x) ((unsigned long)(x) >= PAGE_MASK) +#define BAD_ADDR(x) IS_ERR_VALUE(x) static int set_brk(unsigned long start, unsigned long end) { -
But what about this patch that made the opposite change: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=ce5105... There was a reason for that change... -
Ohhh, interesting. So the original patch has: #define BAD_ADDR(x) ((unsigned long)(x) >= TASK_SIZE) For some reason(?) it got changed to the clearly buggy: #define BAD_ADDR(x) ((unsigned long)(x) >= PAGE_MASK) Jiri's patch undoes that second buggy define, which is very different from the original that was sent in by you and Ernie. -- Politics is the struggle between those who want to make their country the best in the world, and those who believe it already is. Each group calls the other unpatriotic. -
This is a part of execshield patch, fthe pie-compiled binary executable memory layout randomization was extracted from - see http://people.redhat.com/~mingo/exec-shield/exec-shield-nx-2.6.19.patch Note that load_elf_interp() in vanilla kernel differs from the execshield's (and pie-randomization.patch) version. The fix makes the BAD_ADDR check whether the address belongs to the ERR_PTR range, which seems valid for all uses of BAD_ADDR in the patched binfmt_elf.c (do_brk(), elf_map(), do_mmap() etc return valid address or err ptr) ... am I missing something obvious here? -- Jiri Kosina SUSE Labs -
I believe BAD_ADDR macro was changes from ((unsigned long)(x) >= TASK_SIZE)
value of load_elf_interp. All other uses of BAD_ADDR macro are either on
userland addresses (what do_mmap, elf_map, do_brk etc. return;
where TASK_SIZE or more is certainly wrong) or in one case still on unbiased
ELF p_vaddr:
if (BAD_ADDR(k) || elf_ppnt->p_filesz > elf_ppnt->p_memsz ||
in load_elf_binary (where >= TASK_SIZE check is ok too).
So perhaps doing this instead of changing BAD_ADDR to IS_ERR_VAL
might be better:
Signed-off-by: Jakub Jelinek <jakub@redhat.com>
--- linux/fs/binfmt_elf.c 2007-06-08 21:53:45.000000000 +0200
+++ linux/fs/binfmt_elf.c 2007-07-07 14:19:14.000000000 +0200
@@ -80,7 +80,7 @@ static struct linux_binfmt elf_format =
.hasvdso = 1
};
-#define BAD_ADDR(x) ((unsigned long)(x) >= PAGE_MASK)
+#define BAD_ADDR(x) ((unsigned long)(x) >= TASK_SIZE)
static int set_brk(unsigned long start, unsigned long end)
{
@@ -1015,7 +1015,7 @@ static int load_elf_binary(struct linux_
interpreter,
&interp_map_addr,
load_bias);
- if (!BAD_ADDR(elf_entry)) {
+ if (!IS_ERR((void *)elf_entry)) {
/* load_elf_interp() returns relocation adjustment */
interp_load_addr = elf_entry;
elf_entry += loc->interp_elf_ex.e_entry;
Jakub
-
True. But these functions are supposed to perform all the necessary checks and return ERR_PTR if anything fails, so everything is fine and nothing I agree that this is better solution. Andrew, this Jakub's patch should replace the pie-randomization-fix-bad_addr-macro.patch if possible. You can add Signed-off-by: Jiri Kosina <jkosina@suse.cz> if that makes any difference. Thanks, -- Jiri Kosina SUSE Labs -
Hi Jakub, as this raced :) with Andrew who already folded the pie-randomization-fix-bad_addr-macro.patch into pie-randomization.patch, do you think you could rebase this change against the current state of -mm and resend it? Thanks, -- Jiri Kosina SUSE Labs -
Here it is:
Restore BAD_ADDR check strictness, use IS_ERR in the only place where
the stricter BAD_ADDR can't work, as the value is a load bias rather
than userland address.
Signed-off-by: Jakub Jelinek <jakub@redhat.com>
--- linux/fs/binfmt_elf.c 2007-07-10 11:39:29.000000000 +0200
+++ linux/fs/binfmt_elf.c 2007-07-10 11:41:03.000000000 +0200
@@ -80,7 +80,7 @@ static struct linux_binfmt elf_format =
.hasvdso = 1
};
-#define BAD_ADDR(x) IS_ERR_VALUE(x)
+#define BAD_ADDR(x) ((unsigned long)(x) >= TASK_SIZE)
static int set_brk(unsigned long start, unsigned long end)
{
@@ -1005,7 +1005,7 @@ static int load_elf_binary(struct linux_
interpreter,
&interp_map_addr,
load_bias);
- if (!BAD_ADDR(elf_entry)) {
+ if (!IS_ERR((void *)elf_entry)) {
/*
* load_elf_interp() returns relocation
* adjustment
Jakub
-
Signed-off-by: Jiri Kosina <jkosina@suse.cz> Andrew, fold this into pie-randomization.patch if possible please. Thanks, -- Jiri Kosina SUSE Labs -
