| Message ID | 1502103886-19725-1-git-send-email-miles.chen@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.chen@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? Will
On 7 August 2017 at 14:16, Will Deacon <will.deacon@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.chen@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.
On Mon, 2017-08-07 at 14:18 +0100, Ard Biesheuvel wrote: > On 7 August 2017 at 14:16, Will Deacon <will.deacon@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.chen@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? > > The module range is not randomized if CONFIG_KASAN is enabled. (arch/arm64/kernel/kaslr.c) > > 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. The Kernel virtual layout info should be as correct as possible or we should remove the "modules line" in the kernel address layout since it does not show the correct information.
On 7 August 2017 at 14:36, Miles Chen <miles.chen@mediatek.com> wrote: > On Mon, 2017-08-07 at 14:18 +0100, Ard Biesheuvel wrote: >> On 7 August 2017 at 14:16, Will Deacon <will.deacon@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.chen@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? >> > > The module range is not randomized if CONFIG_KASAN is enabled. > (arch/arm64/kernel/kaslr.c) >> >> 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. > > The Kernel virtual layout info should be as correct as possible > or we should remove the "modules line" in the kernel address layout > since it does not show the correct information. > That is your opinion. I disagree.
On Mon, Aug 07, 2017 at 02:18:00PM +0100, Ard Biesheuvel wrote: > On 7 August 2017 at 14:16, Will Deacon <will.deacon@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.chen@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? Will
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.deacon@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.chen@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 Miles > > Will
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.deacon@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.chen@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. > Miles > > > > Will > >
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.deacon@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.chen@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. 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. Will
On 8 August 2017 at 14:19, Will Deacon <will.deacon@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.deacon@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.chen@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.
On Tue, Aug 08, 2017 at 03:04:55PM +0100, Ard Biesheuvel wrote: > On 8 August 2017 at 14:19, Will Deacon <will.deacon@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: > >> > 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. I'm not sure I'd say it works properly in all cases. If we're placing modules in the vmalloc area, then the piece of VA space below that shouldn't be treated as the module area, otherwise we could say that is_vmalloc_or_module_addr() could return 1 for all areas of unused VA space, which I don't think is right. > > 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. Sure, but what use is there in communicating the module region if we don't actually load modules there? It only serves to confuse people IMO. Will
On 8 August 2017 at 15:18, Will Deacon <will.deacon@arm.com> wrote: > On Tue, Aug 08, 2017 at 03:04:55PM +0100, Ard Biesheuvel wrote: >> On 8 August 2017 at 14:19, Will Deacon <will.deacon@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: >> >> > 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. > > I'm not sure I'd say it works properly in all cases. If we're placing > modules in the vmalloc area, then the piece of VA space below that shouldn't > be treated as the module area, otherwise we could say that > is_vmalloc_or_module_addr() could return 1 for all areas of unused VA > space, which I don't think is right. > OK, so what you're after is a is_vmalloc_or_module_addr() that returns false for the module region if we're currently loading modules elsewhere. What does that actually buy us? The module region will either be used for modules, or remain unused. AFAIK the main use case for is_vmalloc_or_module_addr() is to decide whether the addresses is vmapped so that you can set ro or nx attributes. >> > 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. > > Sure, but what use is there in communicating the module region if we don't > actually load modules there? It only serves to confuse people IMO. > Yes, I get that. But as I said, module PLT support may result in modules ending up anywhere in the VMALLOC space, so even if we have a preferred 128 MB window, modules may live elsewhere as well. Having a default window, and actual window *and* modules potentially turning up in other places is even more confusing. So I guess we could drop the 'modules' line in dmesg if module_alloc_base points into the VMALLOC space, because that guarantees us no modules will be allocated in the module region. Other than that, I don't think tinkering with MODULES_VADDR is going to improve things.
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c index 5960bef..57a11d5 100644 --- a/arch/arm64/mm/init.c +++ b/arch/arm64/mm/init.c @@ -52,6 +52,7 @@ #include <asm/sizes.h> #include <asm/tlb.h> #include <asm/alternative.h> +#include <asm/module.h> /* * We need to be able to catch inadvertent references to memstart_addr @@ -609,8 +610,8 @@ void __init mem_init(void) pr_notice(" kasan : 0x%16lx - 0x%16lx (%6ld GB)\n", MLG(KASAN_SHADOW_START, KASAN_SHADOW_END)); #endif - pr_notice(" modules : 0x%16lx - 0x%16lx (%6ld MB)\n", - MLM(MODULES_VADDR, MODULES_END)); + pr_notice(" modules : 0x%16llx - 0x%16llx (%6lld MB)\n", + MLM(module_alloc_base, module_alloc_base + MODULES_VSIZE)); pr_notice(" vmalloc : 0x%16lx - 0x%16lx (%6ld GB)\n", MLG(VMALLOC_START, VMALLOC_END)); pr_notice(" .text : 0x%p" " - 0x%p" " (%6ld KB)\n",
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.chen@mediatek.com> --- arch/arm64/mm/init.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)