On Fri, Feb 1, 2019 at 6:51 PM Cao jin <[email protected]> wrote: > > comments fix: input_size is ZO image size which just don't count .bss > in, but has .text, .data, etc; > drop unecessary alignment: minimum is either 512M or output, both are > CONFIG_PHYSICAL_ALIGN aligned(output is aligned in head_32/64.S). But > mention it in earlier comments. > > Signed-off-by: Cao jin <[email protected]> > --- > arch/x86/boot/compressed/kaslr.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/boot/compressed/kaslr.c > b/arch/x86/boot/compressed/kaslr.c > index 9ed9709d9947..a947c5aba34e 100644 > --- a/arch/x86/boot/compressed/kaslr.c > +++ b/arch/x86/boot/compressed/kaslr.c > @@ -360,7 +360,7 @@ static void handle_mem_options(void) > * (i.e. it does not include its run size). This range must be avoided > * because it contains the data used for decompression. > * > - * [input+input_size, output+init_size) is [_text, _end) for ZO. This > + * [input+input_size, output+init_size) is [_bss, _end) for ZO. This
This isn't right. The comment was correct before. See arch/x86/boot/compressed/vmlinux.lds.S for the layout of the ZO image: after the compressed image is _text, _rodata, _got, _data, _bss, _pgtable, and _end. "[_text, _end)" correctly identifies the span used. > * range includes ZO's heap and stack, and must be avoided since it > * performs the decompression. > * > @@ -763,9 +763,6 @@ static unsigned long find_random_phys_addr(unsigned long > minimum, > return 0; > } > > - /* Make sure minimum is aligned. */ > - minimum = ALIGN(minimum, CONFIG_PHYSICAL_ALIGN); > - I would prefer to keep this runtime calculation since it enforces the requirement instead of making leaving it in a comment. When this goes wrong, you get an unbootable kernel, which is very frustrating to debug. > if (process_efi_entries(minimum, image_size)) > return slots_fetch_random(); > > @@ -831,8 +828,8 @@ void choose_random_location(unsigned long input, > > /* > * Low end of the randomization range should be the > - * smaller of 512M or the initial kernel image > - * location: > + * smaller of 512M or the initial kernel image location. > + * Should be aligned to CONFIG_PHYSICAL_ALIGN. This is fine to mention, sure. -Kees > */ > min_addr = min(*output, 512UL << 20); > > -- > 2.17.0 > > > -- Kees Cook
