| 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: <CAGXu5jK5Pt8cijLKwe6X+0NPUEAXrZJJgxnhp_Ln2V95aVzPsA@mail.gmail.com>
Date: Wed, 5 Jul 2017 12:06:18 -0700
From: Kees Cook <keescook@...omium.org>
To: Baoquan He <bhe@...hat.com>
Cc: LKML <linux-kernel@...r.kernel.org>,
"H. Peter Anvin" <hpa@...or.com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>,
"x86@...nel.org" <x86@...nel.org>,
"Kees Cook <keescook@...omium.org, Yinghai Lu" <yinghai@...nel.org>,
Arnd Bergmann <arnd@...db.de>,
Thomas Garnier <thgarnie@...gle.com>
Subject: Re: [PATCH] x86/boot/KASLR: Skip relocation handling in no kaslr case
On Tue, Jun 27, 2017 at 4:24 PM, Baoquan He <bhe@...hat.com> wrote:
> Hi Kees,
>
> On 06/27/17 at 03:42pm, Kees Cook wrote:
>> On Sat, Jun 24, 2017 at 7:25 AM, Baoquan He <bhe@...hat.com> wrote:
>> > Kdump kernel will reset to firmware after crash is trigered when
>> > crashkernel=xxM,high is added to kernel command line. Kexec has the
>> > same phenomenon. This only happened on system with kaslr code
>> > compiled in and kernel option 'nokaslr'is added. Both of them works
>> > well when kaslr is enabled.
>> >
>> > When crashkernel high is set or kexec case, kexec/kdump kernel will be
>> > put above 4G. Since we assign the original loading address of kernel to
>> > virt_addr as initial value, the virt_addr will be larger than 1G if kaslr
>> > is disabled, it exceeds the kernel mapping size which is only 1G. Then
>> > it will cause relocation handling error in handle_relocations().
>> >
>> > In fact this is a known issue and fixed in commit:
>> >
>> > ... f285f4a ("x86, boot: Skip relocs when load address unchanged")
>> >
>> > But above fix was lost carelessly in later commit:
>> >
>> > ... 8391c73 ("x86/KASLR: Randomize virtual address separately")
>>
>> Hrm, this changes more than just fixing this missing logic. How about
>> just simply:
>
> Below code was added to fix the kexec/kdump kernel with kaslr disabled,
> at that time kernel kaslr physical address and virtual address
> randomization are coupled. What it was doing is to randomize physical
> address in 1G range and add the delta onto the starting address of
> virtual address, 0xffffffff80000000.
>
> But now the physical and virtual address randomization has been
> separated. It means that whether each of them is changed or not
> randomly, the randomization wont' be impacted. So below change you
> provided will has two problems:
Well, it's only sometimes separated. When it's still tied together,
the (identity-mapped) physical randomization _is_ the virtual
randomization.
> 1st, the 'virt_addr' represents the offset of virtual address
> randomization between 0xffffffff80000000 and 0xffffffffc0000000, should
> not get a initial value '(unsigned long)output'. The 'output' could be
> any value between 16M and 64T. It makes no sense to make the assignment
> and could bring confusion to code readers.
>
> 2nd, for x86 64, we do the relocation handling if and only if virtual
> address is randomized to a different value, namely the offset 'virt_addr'
> has a value which is not 16M. You can see this in handle_relocations().
>
> if (IS_ENABLED(CONFIG_X86_64))
> delta = virt_addr - LOAD_PHYSICAL_ADDR;
>
> if (!delta) {
> debug_putstr("No relocation needed... ");
> return;
> }
On x86_64, the kernel is built so that if the virt address matches
LOAD_PHYSICAL_ADDR, we don't have to do relocations.
> Now below code will bring a bug that if physical address randomization fail
> to get a different value, but virtual address randomization may succeed to
> get one, then it won't do relocation handling. This contradicts with the
> design and implementation of the current code.
Let's address this specific issue (below).
>
>>
>> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
>> index b3c5a5f030ce..4496a05d1f8a 100644
>> --- a/arch/x86/boot/compressed/misc.c
>> +++ b/arch/x86/boot/compressed/misc.c
>> @@ -339,6 +339,7 @@ asmlinkage __visible void *extract_kernel(void
>> *rmode, memptr heap,
>> {
>> const unsigned long kernel_total_size = VO__end - VO__text;
>> unsigned long virt_addr = (unsigned long)output;
>> + unsigned long virt_addr_orig = virt_addr;
>>
>> /* Retain x86 boot parameters pointer passed from startup_32/64. */
>> boot_params = rmode;
>> @@ -405,7 +406,12 @@ asmlinkage __visible void *extract_kernel(void
>> *rmode, memptr heap,
>> __decompress(input_data, input_len, NULL, NULL, output, output_len,
>> NULL, error);
>> parse_elf(output);
>> - handle_relocations(output, output_len, virt_addr);
>> + /*
>> + * 32-bit always performs relocations. 64-bit relocations are only
>> + * needed if kASLR has chosen a different load address.
>> + */
>> + if (!IS_ENABLED(CONFIG_X86_64) || virt_addr != virt_addr_orig)
>> + handle_relocations(output, output_len, virt_addr);
>> debug_putstr("done.\nBooting the kernel.\n");
>> return output;
>> }
>>
>> > diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
>> > index b3c5a5f0..c945acd 100644
>> > --- a/arch/x86/boot/compressed/misc.c
>> > +++ b/arch/x86/boot/compressed/misc.c
>> > @@ -338,7 +338,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
>> > unsigned long output_len)
>> > {
>> > const unsigned long kernel_total_size = VO__end - VO__text;
>> > - unsigned long virt_addr = (unsigned long)output;
>> > + unsigned long virt_addr = LOAD_PHYSICAL_ADDR;
>>
>> I don't like this being hard-coded. The logic for the output address
>> is already handled in the .S files, and may not be LOAD_PHYSICAL_ADDR.
>> I'd prefer just adding back the simple test of virt_addr changing.
>
> Do you mean the handling in boot/compressed/head_64.S? Whatever it does,
> it's only for physical address. The virtual address mapping is not
> touched. Here virt_addr respresents the offset between
> 0xffffffff80000000, 0xffffffffc0000000. And if skip the relocation
> handling, we can see that the '_text' will be mapped to
> 0xffffffff81000000 forever, no matter where physical address of '_text'
> is. So here LOAD_PHYSICAL_ADDR is default value of 'virt_addr'.
So, this isn't always true. The output address is the address assigned
by head_*.S for the extraction destination. We can't just change that,
since it's based on where to perform the extraction, etc. All the
logic in there is designed to make sure we're actually >=
LOAD_PHYSICAL_ADDR:
cmpl $LOAD_PHYSICAL_ADDR, %ebx
jge 1f
#endif
movl $LOAD_PHYSICAL_ADDR, %ebx
1:
We can't undo that since the calculation performed there is critical
to getting it into the right place.
>> >
>> > /* Retain x86 boot parameters pointer passed from startup_32/64. */
>> > boot_params = rmode;
>> > @@ -397,7 +397,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
>> > #ifndef CONFIG_RELOCATABLE
>> > if ((unsigned long)output != LOAD_PHYSICAL_ADDR)
>> > error("Destination address does not match LOAD_PHYSICAL_ADDR");
>> > - if ((unsigned long)output != virt_addr)
>> > + if (virt_addr != LOAD_PHYSICAL_ADDR)
>> > error("Destination virtual address changed when not relocatable");
>> > #endif
>> >
>> > diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
>> > index 1c8355e..766a521 100644
>> > --- a/arch/x86/boot/compressed/misc.h
>> > +++ b/arch/x86/boot/compressed/misc.h
>> > @@ -81,8 +81,6 @@ static inline void choose_random_location(unsigned long input,
>> > unsigned long output_size,
>> > unsigned long *virt_addr)
>> > {
>> > - /* No change from existing output location. */
>> > - *virt_addr = *output;
>> > }
>> > #endif
So, the code in question is this:
static void handle_relocations(void *output, unsigned long output_len,
unsigned long virt_addr)
{
int *reloc;
unsigned long delta, map, ptr;
unsigned long min_addr = (unsigned long)output;
unsigned long max_addr = min_addr + (VO___bss_start - VO__text);
/*
* Calculate the delta between where vmlinux was linked to load
* and where it was actually loaded.
*/
delta = min_addr - LOAD_PHYSICAL_ADDR;
/*
* The kernel contains a table of relocation addresses. Those
* addresses have the final load address of the kernel in virtual
* memory. We are currently working in the self map. So we need to
* create an adjustment for kernel memory addresses to the self map.
* This will involve subtracting out the base address of the kernel.
*/
map = delta - __START_KERNEL_map;
/*
* 32-bit always performs relocations. 64-bit relocations are only
* needed if KASLR has chosen a different starting address offset
* from __START_KERNEL_map.
*/
if (IS_ENABLED(CONFIG_X86_64))
delta = virt_addr - LOAD_PHYSICAL_ADDR;
if (!delta) {
debug_putstr("No relocation needed... ");
return;
}
debug_putstr("Performing relocations... ");
The first use of delta is to figure out the correct relocation offset,
which is based on the relocation tables, etc.
The second use is for decoupled phys/virt (x86_64 only), but I don't
think these need to be separate, do they? I think "delta" should only
ever operate on the virt_addr, yes?
in misc.c, this chooses a physical location and a virtual:
choose_random_location((unsigned long)input_data, input_len,
(unsigned long *)&output,
max(output_len, kernel_total_size),
&virt_addr);
Then we place the kernel in the physical location:
__decompress(input_data, input_len, NULL, NULL, output, output_len,
NULL, error);
And finally handle relocations:
handle_relocations(output, output_len, virt_addr);
"output" is the identity-mapped physical (and possibly virtual)
location, and virt_addr is the virtual.
So, it looks to me like the calculations at the top of
handle_relocations() may not be correct for the case you're hitting?
What are the values there in that case? We must not hard-code output,
so handle_relocations() has to be entirely self-contained with its
logic.
I actually think the bug might be that choose_random_location() in the
nokaslr case doesn't explicitly set virt_addr to output, like the
no-op does:
void choose_random_location(unsigned long input,
unsigned long input_size,
unsigned long *output,
unsigned long output_size,
unsigned long *virt_addr)
{
unsigned long random_addr, min_addr;
if (cmdline_find_option_bool("nokaslr")) {
warn("KASLR disabled: 'nokaslr' on cmdline.");
return;
}
Perhaps we need a few fixes:
diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 91f27ab970ef..e1e57c626093 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -627,6 +627,7 @@ void choose_random_location(unsigned long input,
if (cmdline_find_option_bool("nokaslr")) {
warn("KASLR disabled: 'nokaslr' on cmdline.");
+ *virt_addr = *output;
return;
}
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index 00241c815524..3eb754e4f823 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -198,12 +198,13 @@ static void handle_relocations(void *output,
unsigned long output_len,
* needed if KASLR has chosen a different starting address offset
* from __START_KERNEL_map.
*/
- if (IS_ENABLED(CONFIG_X86_64))
+ if (IS_ENABLED(CONFIG_X86_64)) {
delta = virt_addr - LOAD_PHYSICAL_ADDR;
- if (!delta) {
- debug_putstr("No relocation needed... ");
- return;
+ if (!delta) {
+ debug_putstr("No relocation needed... ");
+ return;
+ }
}
debug_putstr("Performing relocations... ");
--
Kees Cook
Pixel Security
Powered by blists - more mailing lists