[PATCH] arm64: correct modules range of kernel virtual memory layout

[PATCH] arm64: correct modules range of kernel virtual memory layout

[Miles Chen]

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(-)

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",
-- 
1.9.1

Re: [PATCH] arm64: correct modules range of kernel virtual memory layout

[Will Deacon]

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

Re: [PATCH] arm64: correct modules range of kernel virtual memory layout

[Ard Biesheuvel]

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.

Re: [PATCH] arm64: correct modules range of kernel virtual memory layout

[Miles Chen]

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.

Re: [PATCH] arm64: correct modules range of kernel virtual memory layout

[Ard Biesheuvel]

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.

Re: [PATCH] arm64: correct modules range of kernel virtual memory layout

[Will Deacon]

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

Re: [PATCH] arm64: correct modules range of kernel virtual memory layout

[Miles Chen]

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

Re: [PATCH] arm64: correct modules range of kernel virtual memory layout

[Miles Chen]

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
> 
> 

Re: [PATCH] arm64: correct modules range of kernel virtual memory layout

[Will Deacon]

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

Re: [PATCH] arm64: correct modules range of kernel virtual memory layout

[Ard Biesheuvel]

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.

Re: [PATCH] arm64: correct modules range of kernel virtual memory layout

[Will Deacon]

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

Re: [PATCH] arm64: correct modules range of kernel virtual memory layout

[Ard Biesheuvel]

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.