On 8 August 2017 at 14:19, Will Deacon <will.dea...@arm.com> wrote: > On Tue, Aug 08, 2017 at 01:27:25PM +0800, Miles Chen wrote: >> On Tue, 2017-08-08 at 12:44 +0800, Miles Chen wrote: >> > On Mon, 2017-08-07 at 15:01 +0100, Will Deacon wrote: >> > > On Mon, Aug 07, 2017 at 02:18:00PM +0100, Ard Biesheuvel wrote: >> > > > On 7 August 2017 at 14:16, Will Deacon <will.dea...@arm.com> wrote: >> > > > > On Mon, Aug 07, 2017 at 07:04:46PM +0800, Miles Chen wrote: >> > > > >> The commit f80fb3a3d508 ("arm64: add support for kernel ASLR") >> > > > >> moved module virtual address to >> > > > >> [module_alloc_base, module_alloc_base + MODULES_VSIZE). >> > > > >> >> > > > >> Display module information of the virtual kernel >> > > > >> memory layout by using module_alloc_base. >> > > > >> >> > > > >> testing output: >> > > > >> 1) Current implementation: >> > > > >> Virtual kernel memory layout: >> > > > >> modules : 0xffffff8000000000 - 0xffffff8008000000 ( 128 >> > > > >> MB) >> > > > >> 2) this patch + KASLR: >> > > > >> Virtual kernel memory layout: >> > > > >> modules : 0xffffff8000560000 - 0xffffff8008560000 ( 128 >> > > > >> MB) >> > > > >> 3) this patch + KASLR and a dummy seed: >> > > > >> Virtual kernel memory layout: >> > > > >> modules : 0xffffffa7df637000 - 0xffffffa7e7637000 ( 128 >> > > > >> MB) >> > > > >> >> > > > >> Signed-off-by: Miles Chen <miles.c...@mediatek.com> >> > > > >> --- >> > > > >> arch/arm64/mm/init.c | 5 +++-- >> > > > >> 1 file changed, 3 insertions(+), 2 deletions(-) >> > > > > >> > > > > Does this mean the modules code in our pt dumper is busted >> > > > > (arch/arm64/mm/dump.c)? Also, what about KASAN, which uses these >> > > > > addresses >> > > > > too (in kasan_init)? Should we just remove MODULES_VADDR and >> > > > > MODULES_END >> > > > > altogether? >> > > > > >> > > > >> > > > I don't think we need this patch. The 'module' line simply prints the >> > > > VA region that is reserved for modules. The fact that we end up >> > > > putting them elsewhere when running randomized does not necessarily >> > > > mean this line should reflect that. >> > > >> > > I was more concerned by other users of MODULES_VADDR tbh, although I see >> > > now that we don't randomize the module region if kasan is enabled. Still, >> > > the kcore code adds the modules region as a separate area (distinct from >> > > vmalloc) if MODULES_VADDR is defined, the page table dumping code uses >> > > MODULES_VADDR to identify the module region and I think we'll get false >> > > positives from is_vmalloc_or_module_addr, which again uses the static >> > > region. >> > > >> > > So, given that MODULES_VADDR never points at the module area, can't we >> > > get >> > > rid of it? >> > >> > Agreed.MODULES_VADDR should be phased out. Considering the kernel >> > modules live somewhere between [VMALLOC_START, VMALLOC_END) now: >> > (arch/arm64/kernel/module.c:module_alloc). I suggest the following >> > changes: >> > >> > 1. is_vmalloc_or_module_addr() should return is_vmalloc_addr() directly >> > 2. arch/arm64/mm/dump.c does not need MODULES_VADDR and MODULES_END. >> > 3. kasan uses [module_alloc_base, module_alloc_base + MODULES_VSIZE) to >> > get the shadow memory? (the kernel modules still live in this range when >> > kasan is enabled) >> > 4. remove modules line in kernel memory layout >> > (optional, thanks for Ard's feedback) >> > 5. remove MODULE_VADDR, MODULES_END definition >> >> I was wrong about this. is_vmalloc_or_module_addr() is defined >> in mm/vmalloc and it uses MODULES_VADDR and MODULES_END. >> May it is better to give MODULES_VADDR and MODULES_END >> proper values, not remove them. > > I think the only cases where the modules area isn't completely contained > within vmalloc is where either randomization is disabled > (CONFIG_RANDOMIZE_BASE=n) or we fail in kaslr_early_init. However, in both > of these cases, module_alloc_base is set correctly, so perhaps we could > defined MODULES_VADDR in terms of that oto get is_vmalloc_or_module_addr > working properly.
is_vmalloc_or_module_addr() already works properly: modules are loaded into their own dedicated region, or in the vmalloc space otherwise. Note that this even applies when disregarding KASRL: the module PLT support uses the vmalloc region as overflow if the module region is exhausted. > We'd need to add some code to the table dumper to avoid > printing the module area if it's contained within vmalloc, and that could > also be used for the kernel memory layout print. > I agree it may be misleading if the module region is empty while modules have been loaded. But let's not conflate the module *region* with the actual locations where modules are loaded: without CONFIG_RANDOMIZE_MODULE_REGION_FULL=y, this is a 128 MB region that overlaps the kernel .text section.