Re: [PATCH v3 5/7] x86, kaslr: Consolidate mem_avoid array filling

Kees Cook
2015-03-10T01:00:07Z

On Sat, Mar 7, 2015 at 2:07 PM, Yinghai Lu <yinghai-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> Now ZO sit end of the buffer, we can find out where is ZO text
> and data/bss etc.
>
> [input, input+input_size) is copied compressed kernel, not the whole ZO.
> [output, output+init_size) is the buffer for VO.
>
> [input+input_size, output+init_size) is [_text, _end) for ZO.
> that could be first range in mem_avoid. We don't need to guess that anymore.
>
> That area already include heap and stack for ZO running. So we don't need
> to put them into mem_avoid array.
>
> We need to put boot_params into the mem_avoid too. As with 64bit bootloader
> could put it anywhere.

This may be a stupid question, but are boot_params being used outside
of the compressed loader? If so, it might make sense to split that
change into a separate patch to go to stable, if it's causing
problems. (And document what problem is getting solved.)

-Kees

>
> Also change output_size referring to init_size, as we pass init_size instead.
>
> Cc: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Yinghai Lu <yinghai-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
> arch/x86/boot/compressed/aslr.c | 29 ++++++++++++++---------------
> arch/x86/boot/compressed/misc.h | 4 ++--
> 2 files changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/aslr.c b/arch/x86/boot/compressed/aslr.c
> index 7083c16..a279514 100644
> --- a/arch/x86/boot/compressed/aslr.c
> +++ b/arch/x86/boot/compressed/aslr.c
> @@ -116,7 +116,7 @@ struct mem_vector {
> unsigned long size;
> };
>
> -#define MEM_AVOID_MAX 5
> +#define MEM_AVOID_MAX 4
> static struct mem_vector mem_avoid[MEM_AVOID_MAX];
>
> static bool mem_contains(struct mem_vector *region, struct mem_vector *item)
> @@ -142,7 +142,7 @@ static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
> }
>
> static void mem_avoid_init(unsigned long input, unsigned long input_size,
> - unsigned long output, unsigned long output_size)
> + unsigned long output, unsigned long init_size)
> {
> u64 initrd_start, initrd_size;
> u64 cmd_line, cmd_line_size;
> @@ -151,10 +151,13 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
>
> /*
> * Avoid the region that is unsafe to overlap during
> - * decompression (see calculations at top of misc.c).
> + * decompression.
> + * As we already move ZO (arch/x86/boot/compressed/vmlinux)
> + * to the end of buffer, [input+input_size, output+init_size)
> + * has [_text, _end) for ZO.
> */
> - unsafe_len = (output_size >> 12) + 32768 + 18;
> - unsafe = (unsigned long)input + input_size - unsafe_len;
> + unsafe_len = output + init_size - (input + input_size);
> + unsafe = (unsigned long)input + input_size;
> mem_avoid[0].start = unsafe;
> mem_avoid[0].size = unsafe_len;
>
> @@ -176,13 +179,9 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
> mem_avoid[2].start = cmd_line;
> mem_avoid[2].size = cmd_line_size;
>
> - /* Avoid heap memory. */
> - mem_avoid[3].start = (unsigned long)free_mem_ptr;
> - mem_avoid[3].size = BOOT_HEAP_SIZE;
> -
> - /* Avoid stack memory. */
> - mem_avoid[4].start = (unsigned long)free_mem_end_ptr;
> - mem_avoid[4].size = BOOT_STACK_SIZE;
> + /* Avoid params */
> + mem_avoid[3].start = (unsigned long)real_mode;
> + mem_avoid[3].size = sizeof(*real_mode);
> }
>
> /* Does this memory vector overlap a known avoided area? */
> @@ -327,7 +326,7 @@ unsigned char *choose_kernel_location(struct boot_params *params,
> unsigned char *input,
> unsigned long input_size,
> unsigned char *output,
> - unsigned long output_size)
> + unsigned long init_size)
> {
> unsigned long choice = (unsigned long)output;
> unsigned long random;
> @@ -349,10 +348,10 @@ unsigned char *choose_kernel_location(struct boot_params *params,
>
> /* Record the various known unsafe memory ranges. */
> mem_avoid_init((unsigned long)input, input_size,
> - (unsigned long)output, output_size);
> + (unsigned long)output, init_size);
>
> /* Walk e820 and find a random address. */
> - random = find_random_addr(choice, output_size);
> + random = find_random_addr(choice, init_size);
> if (!random) {
> debug_putstr("KASLR could not find suitable E820 region...\n");
> goto out;
> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
> index ee3576b..23156e7 100644
> --- a/arch/x86/boot/compressed/misc.h
> +++ b/arch/x86/boot/compressed/misc.h
> @@ -61,7 +61,7 @@ unsigned char *choose_kernel_location(struct boot_params *params,
> unsigned char *input,
> unsigned long input_size,
> unsigned char *output,
> - unsigned long output_size);
> + unsigned long init_size);
> /* cpuflags.c */
> bool has_cpuflag(int flag);
> #else
> @@ -70,7 +70,7 @@ unsigned char *choose_kernel_location(struct boot_params *params,
> unsigned char *input,
> unsigned long input_size,
> unsigned char *output,
> - unsigned long output_size)
> + unsigned long init_size)
> {
> return output;
> }
> --
> 1.8.4.5
>

--
Kees Cook
Chrome OS Security