Message-ID: <20140910072115.GA31685@dhcp-16-116.nay.redhat.com>
Date:	Wed, 10 Sep 2014 15:21:15 +0800
From:	Baoquan He <bhe@...hat.com>
To:	Vivek Goyal <vgoyal@...hat.com>
Cc:	Kees Cook <keescook@...omium.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Andi Kleen <ak@...ux.intel.com>,
	Ingo Molnar <mingo@...hat.com>,
	Thomas Deutschmann <whissi@...ssi.de>,
	Dave Young <dyoung@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	WANG Chao <chaowang@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH 1/4] kaslr: check user's config too when handle
 relocations

On 09/09/14 at 03:28pm, Vivek Goyal wrote:
> On Tue, Sep 09, 2014 at 08:53:29AM -0700, Kees Cook wrote:
 
> > 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.

Yes, kernel only handle 2G or -2G relocation since 32 bit signed
relocations. But for aslr, since the kernel text mapping shares 2G
virtual address space with modules, only 1G relocation can be done. I
took a test, when load kernel at 1G, if not checking if output_orig
and output are equal, it will trigger a BIOS reboot. And with the
restriction checking in process_e820_entry(), the kaslr relocations only
happens inside 1G.

If max_addr is outside 1G, namely CONFIG_RANDOMIZE_BASE_MAX_OFFSET, the
kaslr random kernel location choosing won't happen, then checking if
output_orig is equal to outout in handle_relocations(), if equal nothing
happened. This truly don't need to specify "nokaslr". 

In fact, I think below checking will be clearer and works too.


static void handle_relocations(void *output, unsigned long output_len)
{

...

#if CONFIG_X86_64

or

#if CONFIG_RANDOMIZE_BASE
#ifdef CONFIG_HIBERNATION
	if (!cmdline_find_option_bool("kaslr")) {
               debug_putstr("No relocation needed... ");
               return;
	}
#else
	if (cmdline_find_option_bool("nokaslr")) {
               debug_putstr("No relocation needed... ");
               return;
	}
#endif

	if (max_addr > CONFIG_RANDOMIZE_BASE_MAX_OFFSET) {
                debug_putstr("Random addr is not allowed. No relocation
needed... \n");
                return;
        }

#endif

...
}

> 
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists