Re: [PATCH v4 06/20] x86, boot: Fix run_size calculation

From: Kees Cook
Date: Tue Mar 22 2016 - 16:51:57 EST

Next message: Kees Cook: "Re: [PATCH v4 07/20] x86, kaslr: Clean up useless code related to run_size."
Previous message: Toshi Kani: "Re: [PATCH v2 2/6] x86/mm/pat: Add pat_disable() interface"
In reply to: Baoquan He: "[PATCH v4 06/20] x86, boot: Fix run_size calculation"
Next in thread: Baoquan He: "[PATCH v4 05/20] x86, kaskr: Update the description for decompressor worst case"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Mar 22, 2016 at 12:32 AM, Baoquan He <bhe@xxxxxxxxxx> wrote:
> From: Yinghai Lu <yinghai@xxxxxxxxxx>
>
> Firstly, current run_size is calculated via shell script
> arch/x86/tools/calc_run_size.sh. It gets file offset and mem size of section
> .bss and .brk in vmlinux, then add them as follows:
>
> run_size=$(( $offsetA + $sizeA + $sizeB ))
>
> However this is completely wrong. The offset is the starting address of
> section or segment in elf file. Below is a vmlinux I compiled:
>
> [bhe@x1 linux]$ objdump -h vmlinux
>
> vmlinux: file format elf64-x86-64
>
> Sections:
> Idx Name Size VMA LMA File off Algn
> 27 .bss 00170000 ffffffff81ec8000 0000000001ec8000 012c8000 2**12
> ALLOC
> 28 .brk 00027000 ffffffff82038000 0000000002038000 012c8000 2**0
> ALLOC
>
> Here we can get run_size is 0x145f000.
> 0x012c8000 + 0x00170000 + 0x00027000 = 0x145f000
>
> [bhe@x1 linux]$ readelf -l vmlinux
>
> Elf file type is EXEC (Executable file)
> Entry point 0x1000000
> There are 5 program headers, starting at offset 64
>
> Program Headers:
> Type Offset VirtAddr PhysAddr
> FileSiz MemSiz Flags Align
> LOAD 0x0000000000200000 0xffffffff81000000 0x0000000001000000
> 0x0000000000b5e000 0x0000000000b5e000 R E 200000
> LOAD 0x0000000000e00000 0xffffffff81c00000 0x0000000001c00000
> 0x0000000000145000 0x0000000000145000 RW 200000
> LOAD 0x0000000001000000 0x0000000000000000 0x0000000001d45000
> 0x0000000000018158 0x0000000000018158 RW 200000
> LOAD 0x000000000115e000 0xffffffff81d5e000 0x0000000001d5e000
> 0x000000000016a000 0x0000000000301000 RWE 200000
> NOTE 0x000000000099bcac 0xffffffff8179bcac 0x000000000179bcac
> 0x00000000000001bc 0x00000000000001bc 4
>
> Section to Segment mapping:
> Segment Sections...
> 00 .text .notes __ex_table .rodata __bug_table .pci_fixup .tracedata __ksymtab __ksymtab_gpl __ksymtab_strings __init_rodata __param __modver
> 01 .data .vvar
> 02 .data..percpu
> 03 .init.text .init.data .x86_cpu_dev.init .parainstructions .altinstructions .altinstr_replacement .iommu_table .apicdrivers .exit.text .smp_locks .bss .brk
> 04 .notes
>
> Here we can get the same value as current run_size if we add p_offset
> and p_memsz.
> 0x000000000115e000 + 0x0000000000301000 = 0x145f000
>
> But is it right? Obviously not. We should calculate it using the last LOAD
> program segment like this:
> run_size = phdr->p_paddr + phdr->p_memsz - physical load addr of kernel
> run_size = 0x0000000001d5e000 + 0x0000000000301000 - 0x0000000001000000 = 0x105f000
>
> It's equal to VO_end-VO_text and certainly it's simpler to do.
> _end: 0xffffffff8205f000
> _text:0xffffffff81000000
> run_size = 0xffffffff8205f000 - 0xffffffff81000000 = 0x105f000
>
> Secondly run_size is a simple constant, we don't need to pass it around
> and we already have voffset.h for that. We can share voffset.h between
> misc.c and header.S instead of getting run_size in other ways. Hence in
> this patch, we move voffset.h creation code to boot/compressed/Makefile.
>
> Dependence was:
> boot/header.S ==> boot/voffset.h ==> vmlinux
> boot/header.S ==> compressed/vmlinux ==> compressed/misc.c
> Now become:
> boot/header.S ==> compressed/vmlinux ==> compressed/misc.c ==> boot/voffset.h ==> vmlinux
>
> Use macro in misc.c to replace passed run_size.
>
> Fixes: e6023367d779 ("x86, kaslr: Prevent .bss from overlaping initrd")
> Cc: Junjie Mao <eternal.n08@xxxxxxxxx>
> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> Cc: Josh Triplett <josh@xxxxxxxxxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Yinghai Lu <yinghai@xxxxxxxxxx>
> Signed-off-by: Baoquan He <bhe@xxxxxxxxxx>

Thanks for spending the time to convince me on this. :)

Acked-by: Kees Cook <keescook@xxxxxxxxxxxx>

-Kees

> ---
> v2->v3:
> Adjust the patch log.
> v3->v4:
> Correct calculation errors in patch log that Kees pointed out.
>
> arch/x86/boot/Makefile | 11 +----------
> arch/x86/boot/compressed/Makefile | 12 ++++++++++++
> arch/x86/boot/compressed/misc.c | 3 +++
> 3 files changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/boot/Makefile b/arch/x86/boot/Makefile
> index bd021e5..40f0ae2 100644
> --- a/arch/x86/boot/Makefile
> +++ b/arch/x86/boot/Makefile
> @@ -78,15 +78,6 @@ $(obj)/vmlinux.bin: $(obj)/compressed/vmlinux FORCE
>
> SETUP_OBJS = $(addprefix $(obj)/,$(setup-y))
>
> -sed-voffset := -e 's/^$[0-9a-fA-F]*$ [ABCDGRSTVW] $_text\|_end$$$/\#define VO_\2 0x\1/p'
> -
> -quiet_cmd_voffset = VOFFSET $@
> - cmd_voffset = $(NM) $< | sed -n $(sed-voffset) > $@
> -
> -targets += voffset.h
> -$(obj)/voffset.h: vmlinux FORCE
> - $(call if_changed,voffset)
> -
> sed-zoffset := -e 's/^$[0-9a-fA-F]*$ [ABCDGRSTVW] $startup_32\|startup_64\|efi32_stub_entry\|efi64_stub_entry\|efi_pe_entry\|input_data\|_end\|_ehead\|_text\|z_.*$$$/\#define ZO_\2 0x\1/p'
>
> quiet_cmd_zoffset = ZOFFSET $@
> @@ -98,7 +89,7 @@ $(obj)/zoffset.h: $(obj)/compressed/vmlinux FORCE
>
>
> AFLAGS_header.o += -I$(obj)
> -$(obj)/header.o: $(obj)/voffset.h $(obj)/zoffset.h
> +$(obj)/header.o: $(obj)/zoffset.h
>
> LDFLAGS_setup.elf := -T
> $(obj)/setup.elf: $(src)/setup.ld $(SETUP_OBJS) FORCE
> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> index f9ce75d..d9dedd9 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -41,6 +41,18 @@ LDFLAGS_vmlinux := -T
> hostprogs-y := mkpiggy
> HOST_EXTRACFLAGS += -I$(srctree)/tools/include
>
> +sed-voffset := -e 's/^$[0-9a-fA-F]*$ [ABCDGRSTVW] $_text\|_end$$$/\#define VO_\2 _AC(0x\1,UL)/p'
> +
> +quiet_cmd_voffset = VOFFSET $@
> + cmd_voffset = $(NM) $< | sed -n $(sed-voffset) > $@
> +
> +targets += ../voffset.h
> +
> +$(obj)/../voffset.h: vmlinux FORCE
> + $(call if_changed,voffset)
> +
> +$(obj)/misc.o: $(obj)/../voffset.h
> +
> vmlinux-objs-y := $(obj)/vmlinux.lds $(obj)/head_$(BITS).o $(obj)/misc.o \
> $(obj)/string.o $(obj)/cmdline.o \
> $(obj)/piggy.o $(obj)/cpuflags.o
> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> index e7c6fdf..4d317b7 100644
> --- a/arch/x86/boot/compressed/misc.c
> +++ b/arch/x86/boot/compressed/misc.c
> @@ -11,6 +11,7 @@
>
> #include "misc.h"
> #include "../string.h"
> +#include "../voffset.h"
>
> /* WARNING!!
> * This code is compiled with -fPIC and it is relocated dynamically
> @@ -415,6 +416,8 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap,
> lines = real_mode->screen_info.orig_video_lines;
> cols = real_mode->screen_info.orig_video_cols;
>
> + run_size = VO__end - VO__text;
> +
> console_init();
> debug_putstr("early console in decompress_kernel\n");
>
> --
> 2.5.0
>

--
Kees Cook
Chrome OS & Brillo Security

Next message: Kees Cook: "Re: [PATCH v4 07/20] x86, kaslr: Clean up useless code related to run_size."
Previous message: Toshi Kani: "Re: [PATCH v2 2/6] x86/mm/pat: Add pat_disable() interface"
In reply to: Baoquan He: "[PATCH v4 06/20] x86, boot: Fix run_size calculation"
Next in thread: Baoquan He: "[PATCH v4 05/20] x86, kaskr: Update the description for decompressor worst case"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]