On Mon, Mar 7, 2016 at 9:21 PM, Baoquan He <b...@redhat.com> wrote:
> On 03/07/16 at 03:28pm, Kees Cook wrote:
>> On Fri, Mar 4, 2016 at 8:25 AM, Baoquan He <b...@redhat.com> wrote:
>> > From: Yinghai Lu <ying...@kernel.org>
>> >
>> > We are going to support kaslr with 64bit above 4G, and new random output
>> > could be anywhere. Array mem_avoid is used for kaslr to search new output
>> > address. Current code only track range that is after output+output_size.
>> > So we need to track all ranges instead of just after output+output_size.
>> >
>> > Current code has first entry which is extra bytes before input+input_size,
>> > and it is according to output_size. Other entries are for initrd, cmdline,
>> > and heap/stack for ZO running.
>> >
>> > At first, let's check the first entry that should be in the mem_avoid
>> > array.
>> > Now ZO sit end of the buffer always, we can find out where is text and
>> > data/bss etc of ZO.
>> >
>> > Since init_size >= run_size, and input+input_len >= output+output_len,
>> > here make several assumptions for better presentation by graph:
>> > - init_size > run_size
>> > - input+input_len > output+output_len
>> > - run_size > output_len
>>
>> I would like to see each of these assumptions justified. Why is
>> init_size > run_size, etc?
>> choose_kernel_location's "output_size" is calculated as max(run_size,
>> output_len), so run_size may not be > output_len...
>
> Sure. I will add this case in next post. Thanks a lot.
>>
>> >
>> > 0 output input input+input_len
>> > output+init_size
>> > | | | |
>> > |
>> > |-----|-------------------|--------|------------------|----|------------|----------|
>> > | | |
>> > output+init_size-ZO_INIT_SIZE output+output_len
>> > output+run_size
>> >
>> > [output, output+init_size) is the for decompressing buffer for compressed
>> > kernel.
>> >
>> > [output, output+run_size) is for VO run size.
>> > [output, output+output_len) is (VO (vmlinux after objcopy) plus relocs)
>> >
>> > [output+init_size-ZO_INIT_SIZE, output+init_size) is copied ZO.
>> > [input, input+input_len) is copied compressed (VO (vmlinux after objcopy)
>> > plus relocs), not the ZO.
>> >
>> > [input+input_len, output+init_size) is [_text, _end) for ZO. that could be
>> > first range in mem_avoid. Now the new first entry already includes heap and
>> > stack for ZO running. So no need to put them separately into mem_avoid
>> > array.
>> >
>> > Also [input, input+input_size) need be put in mem_avoid array. It is
>> > adjacent
>> > to new first entry, so merge them.
>>
>> I wonder if this diagram and description should live in a comment with the
>> code.
>
> I think it would be very helpful for people interested in this process.
> Do you think it's ok to put it where init_size is calculated in
> boot/header.S? Or other suitable places?
Let's put it in aslr.c since it's the biggest user of these
calculations? I would tend to avoid putting large comments in a .S
file, but that's just personal preference.
-Kees
>>
>>
>> >
>> > At last we need to put boot_params into the mem_avoid too. As with 64bit
>> > bootloader could put it anywhere.
>> >
>> > After these changes, we have all ranges which need be avoided in mem_avoid
>> > array.
>> >
>> > Cc: Kees Cook <keesc...@chromium.org>
>> > Signed-off-by: Yinghai Lu <ying...@kernel.org>
>> > ---
>> > v2->v3:
>> > Adjust the patch log.
>> >
>> > arch/x86/boot/compressed/aslr.c | 29 +++++++++++++----------------
>> > 1 file changed, 13 insertions(+), 16 deletions(-)
>> >
>> > diff --git a/arch/x86/boot/compressed/aslr.c
>> > b/arch/x86/boot/compressed/aslr.c
>> > index 622aa88..b93be03 100644
>> > --- a/arch/x86/boot/compressed/aslr.c
>> > +++ b/arch/x86/boot/compressed/aslr.c
>> > @@ -109,7 +109,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)
>> > @@ -135,21 +135,22 @@ 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 = real_mode->hdr.init_size;
>> > u64 initrd_start, initrd_size;
>> > u64 cmd_line, cmd_line_size;
>> > - unsigned long unsafe, unsafe_len;
>> > char *ptr;
>> >
>> > /*
>> > * 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;
>> > - mem_avoid[0].start = unsafe;
>> > - mem_avoid[0].size = unsafe_len;
>> > + mem_avoid[0].start = input;
>> > + mem_avoid[0].size = (output + init_size) - input;
>> >
>> > /* Avoid initrd. */
>> > initrd_start = (u64)real_mode->ext_ramdisk_image << 32;
>> > @@ -169,13 +170,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? */
>> > @@ -319,7 +316,7 @@ unsigned char *choose_kernel_location(unsigned char
>> > *input,
>> >
>> > /* Record the various known unsafe memory ranges. */
>> > mem_avoid_init((unsigned long)input, input_size,
>> > - (unsigned long)output, output_size);
>> > + (unsigned long)output);
>> >
>> > /* Walk e820 and find a random address. */
>> > random = find_random_addr(choice, output_size);
>> > --
>> > 2.5.0
>> >
>>
>>
>>
>> --
>> Kees Cook
>> Chrome OS & Brillo Security
--
Kees Cook
Chrome OS & Brillo Security
