linux-kernel - Re: [PATCH v3 05/19] x86, boot: Fix run

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Tue, 8 Mar 2016 10:05:51 -0800
From:	Kees Cook <keescook@...omium.org>
To:	Baoquan He <bhe@...hat.com>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Yinghai Lu <yinghai@...nel.org>,
	"H. Peter Anvin" <hpa@...or.com>, Vivek Goyal <vgoyal@...hat.com>,
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
	Andy Lutomirski <luto@...nel.org>, lasse.collin@...aani.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Dave Young <dyoung@...hat.com>,
	Junjie Mao <eternal.n08@...il.com>,
	Josh Triplett <josh@...htriplett.org>,
	Matt Fleming <matt.fleming@...el.com>
Subject: Re: [PATCH v3 05/19] x86, boot: Fix run_size calculation

On Mon, Mar 7, 2016 at 8:57 PM, Baoquan He <bhe@...hat.com> wrote:
> On 03/07/16 at 03:10pm, Kees Cook wrote:
>> On Fri, Mar 4, 2016 at 8:25 AM, Baoquan He <bhe@...hat.com> wrote:
>> > From: Yinghai Lu <yinghai@...nel.org>
>> >
>> > 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+0x012c8000+0x00027000=0x145f000
>>
>> This example calculation looks wrong to me. run_size is offset + size
>> + size (not offset + offset + size):
>>
>> 0x12c8000+0x17000+0x27000 = 0x1306000
>
> Yeah, please forgive my carelessness. I copied the wrong size of .bss.
> But you also typied the wrong value of .bss size, it should be 0x170000.
> So the result is still right.
>
> 0x12c8000+0x170000+0x27000 = 0x145f000

Hah, whoops. Yeah. Math is hard! :)

>> > [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
>>
>> Segment 03 ends at 0xffffffff81d5e000 + 0x301000 = 0xffffffff8205f000,
>> which does match where .brk ends (0xffffffff82038000 + 0x27000 =
>> 0xffffffff8205f000).
>
> Ah, yes, exactly. They prove it in different way.

Yeah, I'm satisfied that this change is correct. I just got confused
by the earlier example. :)

>
>>
>> >
>> > It's equal to VO_end-VO_text and certainly it's simpler to do.
>> > _end: 0xffffffff8205f000
>> > _text:0xffffffff81000000
>> > run_size = 0xffffffff8205f000-0xffffffff81000000=0x105f000
>>
>> I would agree, it would seem like the existing run_size calculation is
>> 0x247000 too high in this example.
>
> It should be 0x400000 high as you mistakenly input the size of .bss ^_^.
> 0x145f000 - 0x105f000 = 0x400000
>
> Extra 4M is added in this example.

Right. Thanks!

-Kees


-- 
Kees Cook
Chrome OS & Brillo Security