Re: [PATCH v4 07/10] powerpc/fsl_booke/32: randomize the kernel image offset

[Jason Yan]

On 2019/8/6 15:56, Christophe Leroy wrote:
> Le 05/08/2019 Ã 08:43, Jason Yan a ÃcritÂ:
> > After we have the basic support of relocate the kernel in some
> > appropriate place, we can start to randomize the offset now.
> >
> > Entropy is derived from the banner and timer, which will change every
> > build and boot. This not so much safe so additionally the bootloader may
> > pass entropy via the /chosen/kaslr-seed node in device tree.
> >
> > We will use the first 512M of the low memory to randomize the kernel
> > image. The memory will be split in 64M zones. We will use the lower 8
> > bit of the entropy to decide the index of the 64M zone. Then we chose a
> > 16K aligned offset inside the 64M zone to put the kernel in.
> >
> > ÂÂÂÂ KERNELBASE
> >
> > ÂÂÂÂÂÂÂÂ |-->ÂÂ 64MÂÂ <--|
> > ÂÂÂÂÂÂÂÂ |ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |
> > ÂÂÂÂÂÂÂÂ +---------------+ÂÂÂ +----------------+---------------+
> > ÂÂÂÂÂÂÂÂ |ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |....|ÂÂÂ |kernel|ÂÂÂ |ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |
> > ÂÂÂÂÂÂÂÂ +---------------+ÂÂÂ +----------------+---------------+
> > ÂÂÂÂÂÂÂÂ |ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |
> > ÂÂÂÂÂÂÂÂ |----->ÂÂ offsetÂÂÂ <-----|
> >
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ kimage_vaddr
> >
> > We also check if we will overlap with some areas like the dtb area, the
> > initrd area or the crashkernel area. If we cannot find a proper area,
> > kaslr will be disabled and boot from the original kernel.
> >
> > Signed-off-by: Jason Yan <yanaijie@xxxxxxxxxx>
> > Cc: Diana Craciun <diana.craciun@xxxxxxx>
> > Cc: Michael Ellerman <mpe@xxxxxxxxxxxxxx>
> > Cc: Christophe Leroy <christophe.leroy@xxxxxx>
> > Cc: Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>
> > Cc: Paul Mackerras <paulus@xxxxxxxxx>
> > Cc: Nicholas Piggin <npiggin@xxxxxxxxx>
> > Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> > Reviewed-by: Diana Craciun <diana.craciun@xxxxxxx>
> > Tested-by: Diana Craciun <diana.craciun@xxxxxxx>
>
> Reviewed-by: Christophe Leroy <christophe.leroy@xxxxxx>

Thanks for your help,

> One small comment below
>
> > ---
> > Â arch/powerpc/kernel/kaslr_booke.c | 322 +++++++++++++++++++++++++++++-
> > Â 1 file changed, 320 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/kaslr_booke.c 
> > b/arch/powerpc/kernel/kaslr_booke.c
> > index 30f84c0321b2..97250cad71de 100644
> > --- a/arch/powerpc/kernel/kaslr_booke.c
> > +++ b/arch/powerpc/kernel/kaslr_booke.c
> > @@ -23,6 +23,8 @@
> > Â #include <linux/delay.h>
> > Â #include <linux/highmem.h>
> > Â #include <linux/memblock.h>
> > +#include <linux/libfdt.h>
> > +#include <linux/crash_core.h>
> > Â #include <asm/pgalloc.h>
> > Â #include <asm/prom.h>
> > Â #include <asm/io.h>
> > @@ -34,15 +36,329 @@
> > Â #include <asm/machdep.h>
> > Â #include <asm/setup.h>
> > Â #include <asm/paca.h>
> > +#include <asm/kdump.h>
> > Â #include <mm/mmu_decl.h>
> > +#include <generated/compile.h>
> > +#include <generated/utsrelease.h>
> > +
> > +#ifdef DEBUG
> > +#define DBG(fmt...) printk(KERN_ERR fmt)
> > +#else
> > +#define DBG(fmt...)
> > +#endif
> > +
> > +struct regions {
> > +ÂÂÂ unsigned long pa_start;
> > +ÂÂÂ unsigned long pa_end;
> > +ÂÂÂ unsigned long kernel_size;
> > +ÂÂÂ unsigned long dtb_start;
> > +ÂÂÂ unsigned long dtb_end;
> > +ÂÂÂ unsigned long initrd_start;
> > +ÂÂÂ unsigned long initrd_end;
> > +ÂÂÂ unsigned long crash_start;
> > +ÂÂÂ unsigned long crash_end;
> > +ÂÂÂ int reserved_mem;
> > +ÂÂÂ int reserved_mem_addr_cells;
> > +ÂÂÂ int reserved_mem_size_cells;
> > +};
> > Â extern int is_second_reloc;
> > +/* Simplified build-specific string for starting entropy. */
> > +static const char build_str[] = UTS_RELEASE " (" LINUX_COMPILE_BY "@"
> > +ÂÂÂÂÂÂÂ LINUX_COMPILE_HOST ") (" LINUX_COMPILER ") " UTS_VERSION;
> > +
> > +static __init void kaslr_get_cmdline(void *fdt)
> > +{
> > +ÂÂÂ int node = fdt_path_offset(fdt, "/chosen");
> > +
> > +ÂÂÂ early_init_dt_scan_chosen(node, "chosen", 1, boot_command_line);
> > +}
> > +
> > +static unsigned long __init rotate_xor(unsigned long hash, const void 
> > *area,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ size_t size)
> > +{
> > +ÂÂÂ size_t i;
> > +ÂÂÂ unsigned long *ptr = (unsigned long *)area;
>
> As area is a void *, this cast shouldn't be necessary. Or maybe it is 
> necessary because it discards the const ?

It's true the cast is not necessary. The ptr can be made const and then 
remove the cast.

> Christophe