[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 15 Apr 2016 09:47:26 +0200
From: Ingo Molnar <mingo@...nel.org>
To: Kees Cook <keescook@...omium.org>
Cc: Baoquan He <bhe@...hat.com>, Yinghai Lu <yinghai@...nel.org>,
Ard Biesheuvel <ard.biesheuvel@...aro.org>,
Matt Redfearn <matt.redfearn@...tec.com>, x86@...nel.org,
"H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...hat.com>,
Borislav Petkov <bp@...en8.de>, Vivek Goyal <vgoyal@...hat.com>,
Andy Lutomirski <luto@...nel.org>, lasse.collin@...aani.org,
Andrew Morton <akpm@...ux-foundation.org>,
Dave Young <dyoung@...hat.com>, kernel-hardening@...ts.openwall.com,
LKML <linux-kernel@...r.kernel.org>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v5 02/21] x86, KASLR: Handle kernel relocation above 2G
* Kees Cook <keescook@...omium.org> wrote:
> From: Baoquan He <bhe@...hat.com>
>
> When processing the relocation table, the offset used to calculate the
> relocation is an int. This is sufficient for calculating the physical
> address of the relocs entry on 32-bit systems and on 64-bit systems when
> the relocation is under 2G. To handle relocations above 2G (seen in
> situations like kexec, netboot, etc), this offset needs to be calculated
> using a long to avoid wrapping and miscalculating the relocation.
>
> Signed-off-by: Baoquan He <bhe@...hat.com>
> [kees: rewrote changelog]
> Signed-off-by: Kees Cook <keescook@...omium.org>
> ---
> arch/x86/boot/compressed/misc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> index f35ad9eb1bf1..c4477d5f3fff 100644
> --- a/arch/x86/boot/compressed/misc.c
> +++ b/arch/x86/boot/compressed/misc.c
> @@ -295,7 +295,7 @@ static void handle_relocations(void *output, unsigned long output_len)
> * So we work backwards from the end of the decompressed image.
> */
> for (reloc = output + output_len - sizeof(*reloc); *reloc; reloc--) {
> - int extended = *reloc;
> + long extended = *reloc;
> extended += map;
>
> ptr = (unsigned long)extended;
This patch and the code it patches is just plain sloppy. See this cast? This is an
object lesson of why type casts in C are actively dangerous, they have hidden the
32-bit truncation bug you've fixed with this patch.
But the primary bug, the cast, should be fixed! Together with all the other casts
of 'extended'.
Also, the boot code should be reviewed for unnecessary casts, it seems to be a
disease:
triton:~/tip> git grep -cE '\(unsigned.*;$' arch/x86/boot/compressed/
arch/x86/boot/compressed/aslr.c:14
arch/x86/boot/compressed/eboot.c:41
arch/x86/boot/compressed/misc.c:5
arch/x86/boot/compressed/misc.h:2
arch/x86/boot/compressed/mkpiggy.c:1
arch/x86/boot/compressed/string.c:4
For example the type dance and overloaded usage that choose_kernel_location() does
with the 'random' local variable in aslr.c is disgusting:
void choose_kernel_location(unsigned char *input,
unsigned long input_size,
unsigned char **output,
unsigned long output_size,
unsigned char **virt_offset)
{
unsigned long random, min_addr;
*virt_offset = (unsigned char *)LOAD_PHYSICAL_ADDR;
#ifdef CONFIG_HIBERNATION
if (!cmdline_find_option_bool("kaslr")) {
debug_putstr("KASLR disabled by default...\n");
return;
}
#else
if (cmdline_find_option_bool("nokaslr")) {
debug_putstr("KASLR disabled by cmdline...\n");
return;
}
#endif
real_mode->hdr.loadflags |= KASLR_FLAG;
/* Record the various known unsafe memory ranges. */
mem_avoid_init((unsigned long)input, input_size,
(unsigned long)*output);
/* Low end should be the smaller of 512M or initial location. */
min_addr = min((unsigned long)*output, 512UL << 20);
/* Walk e820 and find a random address. */
random = find_random_phy_addr(min_addr, output_size);
if (!random)
debug_putstr("KASLR could not find suitable E820 region...\n");
else {
if ((unsigned long)*output != random) {
fill_pagetable(random, output_size);
switch_pagetable();
*output = (unsigned char *)random;
}
}
/* Pick random virtual address starting from LOAD_PHYSICAL_ADDR. */
if (IS_ENABLED(CONFIG_X86_64))
random = find_random_virt_offset(LOAD_PHYSICAL_ADDR,
output_size);
*virt_offset = (unsigned char *)random;
}
Firstly, 'random' is a libc function name. We generally don't overload those.
Secondly, it's a random what? Variable names should make it plenty obvious. So it
should probably be named 'random_addr'.
Third:
/* Walk e820 and find a random address. */
random = find_random_phy_addr(min_addr, output_size);
yeah, so what that comment tells us we knew already, due to the function name!
What the comment should _really_ talk about is the high level purpose. Something
like: 'Walk the e820 map and find a random free RAM address to which we can still
decompress the whole kernel' would work so much better ...
Fourth, this function has seven (!!) type casts. We can sure do better.
Fifth:
/* Pick random virtual address starting from LOAD_PHYSICAL_ADDR. */
if (IS_ENABLED(CONFIG_X86_64))
random = find_random_virt_offset(LOAD_PHYSICAL_ADDR,
output_size);
*virt_offset = (unsigned char *)random;
So the purpose of this whole function is to pick _two_ random addresses: the
random physical address to place the kernel at, and on x86_64, to also randomize
the kernel virtual address, right? So exactly which comment tells us that it's
about this? Names like 'choose_kernel_location' are singular and are actively
misleading about this ...
... and then I haven't even mentioned small details like the imbalanced curly
braces.
This code sucks, and I'm not surprised at all that it was broken. It should be
improved before we can feature-extend it.
Thanks,
Ingo
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
