Messages in this thread | | | Date | Tue, 9 Sep 2014 14:13:43 -0700 | Subject | Re: [PATCH 1/4] kaslr: check user's config too when handle relocations | From | Kees Cook <> |
| |
On Tue, Sep 9, 2014 at 12:28 PM, Vivek Goyal <vgoyal@redhat.com> wrote: > On Tue, Sep 09, 2014 at 08:53:29AM -0700, Kees Cook wrote: >> On Mon, Sep 8, 2014 at 11:24 PM, Baoquan He <bhe@redhat.com> wrote: >> > On 09/05/14 at 10:11am, Kees Cook wrote: >> >> I don't think this is correct. If you look at a02150610776 ("x86, >> >> relocs: Move ELF relocation handling to C"), we always did relocations >> >> on 32-bit when CONFIG_RELOCATABLE was set, so I think this will fail >> >> badly on 32-bit. 64-bit only needs relocation when >> >> CONFIG_RANDOMIZE_BASE is set, so this is probably what needs to be >> >> tested here instead. I think a better option would be, in >> >> decompress_kernel(), to compare output before and after >> >> choose_kernel_location(). If it's the same on 64-bit, >> >> handle_relocations() can be skipped. (Perhaps pass the before/after to >> >> handle_relocations() and it can perform the logic.) >> >> >> >> -Kees >> > >> > Hi Kees, >> > >> > Checking handle_relocations() again, I just didn't notice it's mandatory >> > to do the relocations handling in i386. So in this function delta is >> > checked to see if it's a kaslr relocation handling. This might be a >> > little confusing. But I am fine with it. >> > >> > Per your comment, you prefer to compare the output before and after >> > choose_kernel_location(). That's also good, Lu Yinghai posted a draft >> > patch in this way before, however the checking and the delta calculation >> > are not correct. I changed that and test all cases, it works well. So >> > do you like this it? If yes I will repost it. >> > >> > From 13471bd838c52a0e143c2aee81e3863cfff585bd Mon Sep 17 00:00:00 2001 >> > From: Baoquan He <bhe@redhat.com> >> > Date: Mon, 25 Aug 2014 14:57:43 +0800 >> > Subject: [PATCH] kaslr: check if kernel location is changed >> > >> > Signed-off-by: Baoquan He <bhe@redhat.com> >> > --- >> > arch/x86/boot/compressed/misc.c | 15 +++++++++++---- >> > 1 file changed, 11 insertions(+), 4 deletions(-) >> > >> > diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c >> > index 57ab74d..887f404 100644 >> > --- a/arch/x86/boot/compressed/misc.c >> > +++ b/arch/x86/boot/compressed/misc.c >> > @@ -230,8 +230,9 @@ static void error(char *x) >> > asm("hlt"); >> > } >> > >> > -#if CONFIG_X86_NEED_RELOCS >> > -static void handle_relocations(void *output, unsigned long output_len) >> > +#ifdef CONFIG_X86_NEED_RELOCS >> > +static void handle_relocations(void *output_orig, void *output, >> > + unsigned long output_len) >> > { >> > int *reloc; >> > unsigned long delta, map, ptr; >> > @@ -242,6 +243,9 @@ static void handle_relocations(void *output, unsigned long output_len) >> > * Calculate the delta between where vmlinux was linked to load >> > * and where it was actually loaded. >> > */ >> > + if (output_orig == output) >> > + return; >> > + >> >> I still think this needs a test for the 32-bit case, since IUIC, it >> requires relocations unconditionally. > > [ CC hpa ] > > Bao, for modifications in this area, I would recommend CC hpa too. > > I also think that i386 will always require relocation handling. That's > how Eric had designed it. There was no separate kernel text mapping > region so PAGE_OFFSET was constant. Hence if you shift kernel in physical > address space, you had to shift mappings in virtual address space by > equal amount. > > But in x86_64, we have kernel text mapped in a separate virtual region, and > that allowed us wiggling room and we could load kernel anywhere > in physical address space and just change mappings of kernel text > virtual address region accordingly. > > So I agree that on i386, we will most likely require relocations all > the time. Having said that, it is interesting that one can disable > X86_NEED_RELOCS on i386 while RELOCATBALE=y. > > # Relocation on x86 needs some additional build support > config X86_NEED_RELOCS > def_bool y > depends on RANDOMIZE_BASE || (X86_32 && RELOCATABLE) > > I am not sure how i386 RELOCATABLE kernel will work if X86_NEED_RELOCS=n. > > > Secondly, IIUC, kernel has 32bit signed relocations. That means > relocations can be processed successfully only if kernel is loaded > in first 2G or -2G. If that's the case, then aslr mechanism should > see that where kernel is loaded physically and if it is loaded outside > the range where relocations can be processed successfully, it should > disable itself and output a message.
Yeah, I think this is the best idea. I'm not sure if it can currently handle -2G, though. Right now it gives up doing ASLR if it's above 2G, but doesn't give up on doing relocation, so that's the core of this bug, I think.
-Kees
> > That way, one will not have to pass explicit "nokaslr" parameter to kernel. > If kernel can't handle relocations successfully, it will automatically > disable kaslr. > > Thanks > Vivek > >> >> -Kees >> >> > delta = min_addr - LOAD_PHYSICAL_ADDR; >> > if (!delta) { >> > debug_putstr("No relocation needed... "); >> > @@ -299,7 +303,8 @@ static void handle_relocations(void *output, unsigned long output_len) >> > #endif >> > } >> > #else >> > -static inline void handle_relocations(void *output, unsigned long output_len) >> > +static inline void handle_relocations(void *output_orig, void *output, >> > + unsigned long output_len) >> > { } >> > #endif >> > >> > @@ -360,6 +365,8 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap, >> > unsigned char *output, >> > unsigned long output_len) >> > { >> > + unsigned char *output_orig = output; >> > + >> > real_mode = rmode; >> > >> > sanitize_boot_params(real_mode); >> > @@ -402,7 +409,7 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap, >> > debug_putstr("\nDecompressing Linux... "); >> > decompress(input_data, input_len, NULL, NULL, output, NULL, error); >> > parse_elf(output); >> > - handle_relocations(output, output_len); >> > + handle_relocations(output_orig, output, output_len); >> > debug_putstr("done.\nBooting the kernel.\n"); >> > return output; >> > } >> > -- >> > 1.8.5.3 >> > >> >> >> >> -- >> Kees Cook >> Chrome OS Security
-- Kees Cook Chrome OS Security
| |