Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1379517
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH v5 02/21] x86, KASLR: Handle kernel relocation above 2G |
| Date | 2016-04-15 09:50 +0200 |
| Message-ID | <ro6FP-7VI-7@gated-at.bofh.it> (permalink) |
| References | <rnXVT-YO-3@gated-at.bofh.it> <rnY5A-14p-25@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
* Kees Cook <keescook@chromium.org> wrote:
> From: Baoquan He <bhe@redhat.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@redhat.com>
> [kees: rewrote changelog]
> Signed-off-by: Kees Cook <keescook@chromium.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
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[PATCH v5 00/21] x86, boot: KASLR cleanup and 64-bit improvements Kees Cook <keescook@chromium.org> - 2016-04-15 00:30 +0200
[PATCH v5 05/21] x86, boot: Calculate decompression size during boot not build Kees Cook <keescook@chromium.org> - 2016-04-15 00:30 +0200
Re: [PATCH v5 05/21] x86, boot: Calculate decompression size during boot not build Ingo Molnar <mingo@kernel.org> - 2016-04-15 10:20 +0200
Re: [PATCH v5 05/21] x86, boot: Calculate decompression size during boot not build Kees Cook <keescook@chromium.org> - 2016-04-15 21:20 +0200
[PATCH v5 21/21] x86, KASLR: Allow randomization below load address Kees Cook <keescook@chromium.org> - 2016-04-15 00:30 +0200
[PATCH v5 09/21] x86, KASLR: Correctly bounds-check relocations Kees Cook <keescook@chromium.org> - 2016-04-15 00:40 +0200
[PATCH v5 20/21] x86, KASLR: Remove unused slot tracking code Kees Cook <keescook@chromium.org> - 2016-04-15 00:40 +0200
[PATCH v5 01/21] x86, KASLR: Remove unneeded boot_params argument Kees Cook <keescook@chromium.org> - 2016-04-15 00:40 +0200
Re: [PATCH v5 01/21] x86, KASLR: Remove unneeded boot_params argument Ingo Molnar <mingo@kernel.org> - 2016-04-15 09:30 +0200
Re: [PATCH v5 01/21] x86, KASLR: Remove unneeded boot_params argument Kees Cook <keescook@chromium.org> - 2016-04-15 21:00 +0200
[PATCH v5 19/21] x86, KASLR: Add physical address randomization >4G Kees Cook <keescook@chromium.org> - 2016-04-15 00:40 +0200
[PATCH v5 13/21] x86, boot: Report overlap failures in memcpy Kees Cook <keescook@chromium.org> - 2016-04-15 00:40 +0200
Re: [PATCH v5 13/21] x86, boot: Report overlap failures in memcpy Lasse Collin <lasse.collin@tukaani.org> - 2016-04-15 17:00 +0200
Re: [PATCH v5 13/21] x86, boot: Report overlap failures in memcpy Kees Cook <keescook@chromium.org> - 2016-04-15 21:30 +0200
[PATCH v5 02/21] x86, KASLR: Handle kernel relocation above 2G Kees Cook <keescook@chromium.org> - 2016-04-15 00:40 +0200
Re: [PATCH v5 02/21] x86, KASLR: Handle kernel relocation above 2G Ingo Molnar <mingo@kernel.org> - 2016-04-15 09:50 +0200
Re: [PATCH v5 02/21] x86, KASLR: Handle kernel relocation above 2G Kees Cook <keescook@chromium.org> - 2016-04-15 21:10 +0200
[PATCH v5 16/21] x86, KASLR: Add virtual address choosing function Kees Cook <keescook@chromium.org> - 2016-04-15 00:40 +0200
[PATCH v5 15/21] x86, KASLR: Add slot_area support functions Kees Cook <keescook@chromium.org> - 2016-04-15 00:40 +0200
[PATCH v5 04/21] x86, boot: Move compressed kernel to end of decompression buffer Kees Cook <keescook@chromium.org> - 2016-04-15 00:40 +0200
Re: [PATCH v5 04/21] x86, boot: Move compressed kernel to end of decompression buffer Ingo Molnar <mingo@kernel.org> - 2016-04-15 10:20 +0200
Re: [PATCH v5 04/21] x86, boot: Move compressed kernel to end of decompression buffer Kees Cook <keescook@chromium.org> - 2016-04-18 19:00 +0200
Re: [PATCH v5 04/21] x86, boot: Move compressed kernel to end of decompression buffer Ingo Molnar <mingo@kernel.org> - 2016-04-15 11:10 +0200
[PATCH v5 12/21] x86, 64bit: Set ident_mapping for KASLR Kees Cook <keescook@chromium.org> - 2016-04-15 00:40 +0200
[PATCH v5 08/21] x86, KASLR: Clean up unused code from old run_size Kees Cook <keescook@chromium.org> - 2016-04-15 00:40 +0200
[PATCH v5 14/21] x86, KASLR: Add slot_area to manage random slots Kees Cook <keescook@chromium.org> - 2016-04-15 00:40 +0200
[PATCH v5 17/21] x86, KASLR: Clarify purpose of each get_random_long Kees Cook <keescook@chromium.org> - 2016-04-15 00:40 +0200
[PATCH v5 11/21] x86, boot: Split out kernel_ident_mapping_init Kees Cook <keescook@chromium.org> - 2016-04-15 00:40 +0200
[PATCH v5 10/21] x86, KASLR: Consolidate mem_avoid entries Kees Cook <keescook@chromium.org> - 2016-04-15 00:40 +0200
[PATCH v5 03/21] x86, KASLR: Drop CONFIG_RANDOMIZE_BASE_MAX_OFFSET Kees Cook <keescook@chromium.org> - 2016-04-15 00:40 +0200
Re: [PATCH v5 03/21] x86, KASLR: Drop CONFIG_RANDOMIZE_BASE_MAX_OFFSET Ingo Molnar <mingo@kernel.org> - 2016-04-15 10:10 +0200
Re: [PATCH v5 03/21] x86, KASLR: Drop CONFIG_RANDOMIZE_BASE_MAX_OFFSET Kees Cook <keescook@chromium.org> - 2016-04-15 21:20 +0200
Re: [PATCH v5 03/21] x86, KASLR: Drop CONFIG_RANDOMIZE_BASE_MAX_OFFSET Ingo Molnar <mingo@kernel.org> - 2016-04-16 10:50 +0200
[PATCH v5 06/21] x86, KASLR: Update description for decompressor worst case size Kees Cook <keescook@chromium.org> - 2016-04-15 00:40 +0200
Re: [PATCH v5 06/21] x86, KASLR: Update description for decompressor worst case size Lasse Collin <lasse.collin@tukaani.org> - 2016-04-15 18:20 +0200
[PATCH v5 07/21] x86, boot: Fix run_size calculation Kees Cook <keescook@chromium.org> - 2016-04-15 00:40 +0200
Re: [PATCH v5 07/21] x86, boot: Fix run_size calculation Ingo Molnar <mingo@kernel.org> - 2016-04-15 10:40 +0200
Re: [PATCH v5 07/21] x86, boot: Fix run_size calculation Kees Cook <keescook@chromium.org> - 2016-04-15 21:30 +0200
Re: [PATCH v5 07/21] x86, boot: Fix run_size calculation Ingo Molnar <mingo@kernel.org> - 2016-04-16 11:10 +0200
csiph-web