Re: [PATCH v3 08/19] x86, kaslr: Consolidate mem_avoid array filling

[Baoquan He <bhe@redhat.com>]

On 03/08/16 at 10:17am, Kees Cook wrote:
> On Mon, Mar 7, 2016 at 9:21 PM, Baoquan He <bhe@redhat.com> wrote:
> > On 03/07/16 at 03:28pm, Kees Cook wrote:
> >> On Fri, Mar 4, 2016 at 8:25 AM, Baoquan He <bhe@redhat.com> wrote:
> >> > From: Yinghai Lu <yinghai@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.

Yeah, agree. Will put it in aslr.c.

Thanks a lot for your good suggestions.

>