diff mbox series

[RFC,6/8] riscv/kaslr: clear the original kernel image

Message ID 8373a9d18958b99b72ed9499786dffe45adf9617.1584352425.git.zong.li@sifive.com (mailing list archive)
State New, archived
Headers show
Series Support KASLR for RISC-V | expand

Commit Message

Zong Li March 24, 2020, 7:30 a.m. UTC 
After completing final page table, we can clear original kernel image
and remove executable permission.

Signed-off-by: Zong Li <zong.li@sifive.com>
---
 arch/riscv/include/asm/kaslr.h | 12 ++++++++++++
 arch/riscv/kernel/kaslr.c      | 12 ++++++++++++
 arch/riscv/mm/init.c           |  6 ++++++
 3 files changed, 30 insertions(+)
 create mode 100644 arch/riscv/include/asm/kaslr.h

Comments

Alexandre Ghiti April 7, 2020, 5:11 a.m. UTC | #1 
On 3/24/20 3:30 AM, Zong Li wrote:
> After completing final page table, we can clear original kernel image
> and remove executable permission.
> 
> Signed-off-by: Zong Li <zong.li@sifive.com>
> ---
>   arch/riscv/include/asm/kaslr.h | 12 ++++++++++++
>   arch/riscv/kernel/kaslr.c      | 12 ++++++++++++
>   arch/riscv/mm/init.c           |  6 ++++++
>   3 files changed, 30 insertions(+)
>   create mode 100644 arch/riscv/include/asm/kaslr.h
> 
> diff --git a/arch/riscv/include/asm/kaslr.h b/arch/riscv/include/asm/kaslr.h
> new file mode 100644
> index 000000000000..b165fe71dd4a
> --- /dev/null
> +++ b/arch/riscv/include/asm/kaslr.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2020 SiFive
> + * Copyright (C) 2020 Zong Li <zong.li@sifive.com>
> + */
> +
> +#ifndef _ASM_RISCV_KASLR_H
> +#define _ASM_RISCV_KASLR_H
> +
> +void __init kaslr_late_init(void);
> +
> +#endif /* _ASM_RISCV_KASLR_H */
> diff --git a/arch/riscv/kernel/kaslr.c b/arch/riscv/kernel/kaslr.c
> index 59001d6fdfc3..0bd30831c455 100644
> --- a/arch/riscv/kernel/kaslr.c
> +++ b/arch/riscv/kernel/kaslr.c
> @@ -356,6 +356,18 @@ static __init uintptr_t get_random_offset(u64 seed, uintptr_t kernel_size)
>   	return get_legal_offset(random, kernel_size_align);
>   }
>   
> +void __init kaslr_late_init(void)
> +{
> +	uintptr_t kernel_size;
> +
> +	/* Clear original kernel image. */
> +	if (kaslr_offset) {
> +		kernel_size = (uintptr_t) _end - (uintptr_t) _start;

kernel_size = (uintptr_t) _end - (uintptr_t) _start + 1;

> +		memset((void *)PAGE_OFFSET, 0, kernel_size);

I have been thinking again about our discussion regarding PAGE_OFFSET: 
PAGE_OFFSET actually points to the address where the kernel was loaded, 
not the beginning of memory, that's a bit weird.

Just saying that here, because it took me a few seconds to remember that 
and understand what you were doing here.

> +		set_memory_nx(PAGE_OFFSET, kaslr_offset >> PAGE_SHIFT);

Again, I certainly missed something but when do you use old kernel 
mappings ?

> +	}
> +}
> +
>   uintptr_t __init kaslr_early_init(void)
>   {
>   	u64 seed;
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index 34c6ecf2c599..08e2ce170533 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -15,6 +15,7 @@
>   #include <linux/set_memory.h>
>   #ifdef CONFIG_RELOCATABLE
>   #include <linux/elf.h>
> +#include <asm/kaslr.h>
>   #endif
>   
>   #include <asm/fixmap.h>
> @@ -649,6 +650,11 @@ static void __init setup_vm_final(void)
>   	/* Move to swapper page table */
>   	csr_write(CSR_SATP, PFN_DOWN(__pa_symbol(swapper_pg_dir)) | SATP_MODE);
>   	local_flush_tlb_all();
> +
> +#ifdef CONFIG_RANDOMIZE_BASE
> +	/* Clear orignial kernel image and set the right permission. */
> +	kaslr_late_init();
> +#endif
>   }
>   
>   void free_initmem(void)
> 

Alex
Zong Li April 7, 2020, 11:18 a.m. UTC | #2 
On Tue, Apr 7, 2020 at 1:11 PM Alex Ghiti <alex@ghiti.fr> wrote:
>
> On 3/24/20 3:30 AM, Zong Li wrote:
> > After completing final page table, we can clear original kernel image
> > and remove executable permission.
> >
> > Signed-off-by: Zong Li <zong.li@sifive.com>
> > ---
> >   arch/riscv/include/asm/kaslr.h | 12 ++++++++++++
> >   arch/riscv/kernel/kaslr.c      | 12 ++++++++++++
> >   arch/riscv/mm/init.c           |  6 ++++++
> >   3 files changed, 30 insertions(+)
> >   create mode 100644 arch/riscv/include/asm/kaslr.h
> >
> > diff --git a/arch/riscv/include/asm/kaslr.h b/arch/riscv/include/asm/kaslr.h
> > new file mode 100644
> > index 000000000000..b165fe71dd4a
> > --- /dev/null
> > +++ b/arch/riscv/include/asm/kaslr.h
> > @@ -0,0 +1,12 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (C) 2020 SiFive
> > + * Copyright (C) 2020 Zong Li <zong.li@sifive.com>
> > + */
> > +
> > +#ifndef _ASM_RISCV_KASLR_H
> > +#define _ASM_RISCV_KASLR_H
> > +
> > +void __init kaslr_late_init(void);
> > +
> > +#endif /* _ASM_RISCV_KASLR_H */
> > diff --git a/arch/riscv/kernel/kaslr.c b/arch/riscv/kernel/kaslr.c
> > index 59001d6fdfc3..0bd30831c455 100644
> > --- a/arch/riscv/kernel/kaslr.c
> > +++ b/arch/riscv/kernel/kaslr.c
> > @@ -356,6 +356,18 @@ static __init uintptr_t get_random_offset(u64 seed, uintptr_t kernel_size)
> >       return get_legal_offset(random, kernel_size_align);
> >   }
> >
> > +void __init kaslr_late_init(void)
> > +{
> > +     uintptr_t kernel_size;
> > +
> > +     /* Clear original kernel image. */
> > +     if (kaslr_offset) {
> > +             kernel_size = (uintptr_t) _end - (uintptr_t) _start;
>
> kernel_size = (uintptr_t) _end - (uintptr_t) _start + 1;

OK, change it in the next version. Thanks.

>
> > +             memset((void *)PAGE_OFFSET, 0, kernel_size);
>
> I have been thinking again about our discussion regarding PAGE_OFFSET:
> PAGE_OFFSET actually points to the address where the kernel was loaded,
> not the beginning of memory, that's a bit weird.
>
> Just saying that here, because it took me a few seconds to remember that
> and understand what you were doing here.

In non-kaslr case, we load the kernel to PAGE_OFFSET which points to,
so we clear the old kernel image through PAGE_OFFSET here. Certainly,
we could use a symbol to record the start address of the old kernel
image instead of PAGE_OFFSET here. I don't see other architectures
changing PAGE_OFFSET after copying the kernel to the new location in
kaslr. If you think the PAGE_OFFSET needs to be changed, we need to
give another way to make the page table could create the mappings for
the whole memory and memblock/buddy system could see the whole memory
after the kernel moves.

>
> > +             set_memory_nx(PAGE_OFFSET, kaslr_offset >> PAGE_SHIFT);
>
> Again, I certainly missed something but when do you use old kernel
> mappings ?

We use old kernel mappings when KASLR calculates the random offset, at
that moment, kernel is running on old kernel location.

>
> > +     }
> > +}
> > +
> >   uintptr_t __init kaslr_early_init(void)
> >   {
> >       u64 seed;
> > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > index 34c6ecf2c599..08e2ce170533 100644
> > --- a/arch/riscv/mm/init.c
> > +++ b/arch/riscv/mm/init.c
> > @@ -15,6 +15,7 @@
> >   #include <linux/set_memory.h>
> >   #ifdef CONFIG_RELOCATABLE
> >   #include <linux/elf.h>
> > +#include <asm/kaslr.h>
> >   #endif
> >
> >   #include <asm/fixmap.h>
> > @@ -649,6 +650,11 @@ static void __init setup_vm_final(void)
> >       /* Move to swapper page table */
> >       csr_write(CSR_SATP, PFN_DOWN(__pa_symbol(swapper_pg_dir)) | SATP_MODE);
> >       local_flush_tlb_all();
> > +
> > +#ifdef CONFIG_RANDOMIZE_BASE
> > +     /* Clear orignial kernel image and set the right permission. */
> > +     kaslr_late_init();
> > +#endif
> >   }
> >
> >   void free_initmem(void)
> >
>
> Alex
Alexandre Ghiti April 9, 2020, 5:53 a.m. UTC | #3 
On 4/7/20 7:18 AM, Zong Li wrote:
> On Tue, Apr 7, 2020 at 1:11 PM Alex Ghiti <alex@ghiti.fr> wrote:
>>
>> On 3/24/20 3:30 AM, Zong Li wrote:
>>> After completing final page table, we can clear original kernel image
>>> and remove executable permission.
>>>
>>> Signed-off-by: Zong Li <zong.li@sifive.com>
>>> ---
>>>    arch/riscv/include/asm/kaslr.h | 12 ++++++++++++
>>>    arch/riscv/kernel/kaslr.c      | 12 ++++++++++++
>>>    arch/riscv/mm/init.c           |  6 ++++++
>>>    3 files changed, 30 insertions(+)
>>>    create mode 100644 arch/riscv/include/asm/kaslr.h
>>>
>>> diff --git a/arch/riscv/include/asm/kaslr.h b/arch/riscv/include/asm/kaslr.h
>>> new file mode 100644
>>> index 000000000000..b165fe71dd4a
>>> --- /dev/null
>>> +++ b/arch/riscv/include/asm/kaslr.h
>>> @@ -0,0 +1,12 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Copyright (C) 2020 SiFive
>>> + * Copyright (C) 2020 Zong Li <zong.li@sifive.com>
>>> + */
>>> +
>>> +#ifndef _ASM_RISCV_KASLR_H
>>> +#define _ASM_RISCV_KASLR_H
>>> +
>>> +void __init kaslr_late_init(void);
>>> +
>>> +#endif /* _ASM_RISCV_KASLR_H */
>>> diff --git a/arch/riscv/kernel/kaslr.c b/arch/riscv/kernel/kaslr.c
>>> index 59001d6fdfc3..0bd30831c455 100644
>>> --- a/arch/riscv/kernel/kaslr.c
>>> +++ b/arch/riscv/kernel/kaslr.c
>>> @@ -356,6 +356,18 @@ static __init uintptr_t get_random_offset(u64 seed, uintptr_t kernel_size)
>>>        return get_legal_offset(random, kernel_size_align);
>>>    }
>>>
>>> +void __init kaslr_late_init(void)
>>> +{
>>> +     uintptr_t kernel_size;
>>> +
>>> +     /* Clear original kernel image. */
>>> +     if (kaslr_offset) {
>>> +             kernel_size = (uintptr_t) _end - (uintptr_t) _start;
>>
>> kernel_size = (uintptr_t) _end - (uintptr_t) _start + 1;
> 
> OK, change it in the next version. Thanks.
> 
>>
>>> +             memset((void *)PAGE_OFFSET, 0, kernel_size);
>>
>> I have been thinking again about our discussion regarding PAGE_OFFSET:
>> PAGE_OFFSET actually points to the address where the kernel was loaded,
>> not the beginning of memory, that's a bit weird.
>>
>> Just saying that here, because it took me a few seconds to remember that
>> and understand what you were doing here.
> 
> In non-kaslr case, we load the kernel to PAGE_OFFSET which points to,
> so we clear the old kernel image through PAGE_OFFSET here. Certainly,
> we could use a symbol to record the start address of the old kernel
> image instead of PAGE_OFFSET here. I don't see other architectures
> changing PAGE_OFFSET after copying the kernel to the new location in
> kaslr. If you think the PAGE_OFFSET needs to be changed, we need to
> give another way to make the page table could create the mappings for
> the whole memory and memblock/buddy system could see the whole memory
> after the kernel moves.
>  >>
>>> +             set_memory_nx(PAGE_OFFSET, kaslr_offset >> PAGE_SHIFT);
>>
>> Again, I certainly missed something but when do you use old kernel
>> mappings ?
> 
> We use old kernel mappings when KASLR calculates the random offset, at
> that moment, kernel is running on old kernel location.

Yes but haven't you already cleared the page table from the mappings for 
the old kernel in clear_page_tables called in setup_vm of the new kernel ?

Alex

> 
>>
>>> +     }
>>> +}
>>> +
>>>    uintptr_t __init kaslr_early_init(void)
>>>    {
>>>        u64 seed;
>>> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
>>> index 34c6ecf2c599..08e2ce170533 100644
>>> --- a/arch/riscv/mm/init.c
>>> +++ b/arch/riscv/mm/init.c
>>> @@ -15,6 +15,7 @@
>>>    #include <linux/set_memory.h>
>>>    #ifdef CONFIG_RELOCATABLE
>>>    #include <linux/elf.h>
>>> +#include <asm/kaslr.h>
>>>    #endif
>>>
>>>    #include <asm/fixmap.h>
>>> @@ -649,6 +650,11 @@ static void __init setup_vm_final(void)
>>>        /* Move to swapper page table */
>>>        csr_write(CSR_SATP, PFN_DOWN(__pa_symbol(swapper_pg_dir)) | SATP_MODE);
>>>        local_flush_tlb_all();
>>> +
>>> +#ifdef CONFIG_RANDOMIZE_BASE
>>> +     /* Clear orignial kernel image and set the right permission. */
>>> +     kaslr_late_init();
>>> +#endif
>>>    }
>>>
>>>    void free_initmem(void)
>>>
>>
>> Alex
Alexandre Ghiti April 9, 2020, 8:14 a.m. UTC | #4 
On 4/9/20 1:53 AM, Alex Ghiti wrote:
> 
> 
> On 4/7/20 7:18 AM, Zong Li wrote:
>> On Tue, Apr 7, 2020 at 1:11 PM Alex Ghiti <alex@ghiti.fr> wrote:
>>>
>>> On 3/24/20 3:30 AM, Zong Li wrote:
>>>> After completing final page table, we can clear original kernel image
>>>> and remove executable permission.
>>>>
>>>> Signed-off-by: Zong Li <zong.li@sifive.com>
>>>> ---
>>>>    arch/riscv/include/asm/kaslr.h | 12 ++++++++++++
>>>>    arch/riscv/kernel/kaslr.c      | 12 ++++++++++++
>>>>    arch/riscv/mm/init.c           |  6 ++++++
>>>>    3 files changed, 30 insertions(+)
>>>>    create mode 100644 arch/riscv/include/asm/kaslr.h
>>>>
>>>> diff --git a/arch/riscv/include/asm/kaslr.h 
>>>> b/arch/riscv/include/asm/kaslr.h
>>>> new file mode 100644
>>>> index 000000000000..b165fe71dd4a
>>>> --- /dev/null
>>>> +++ b/arch/riscv/include/asm/kaslr.h
>>>> @@ -0,0 +1,12 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>> +/*
>>>> + * Copyright (C) 2020 SiFive
>>>> + * Copyright (C) 2020 Zong Li <zong.li@sifive.com>
>>>> + */
>>>> +
>>>> +#ifndef _ASM_RISCV_KASLR_H
>>>> +#define _ASM_RISCV_KASLR_H
>>>> +
>>>> +void __init kaslr_late_init(void);
>>>> +
>>>> +#endif /* _ASM_RISCV_KASLR_H */
>>>> diff --git a/arch/riscv/kernel/kaslr.c b/arch/riscv/kernel/kaslr.c
>>>> index 59001d6fdfc3..0bd30831c455 100644
>>>> --- a/arch/riscv/kernel/kaslr.c
>>>> +++ b/arch/riscv/kernel/kaslr.c
>>>> @@ -356,6 +356,18 @@ static __init uintptr_t get_random_offset(u64 
>>>> seed, uintptr_t kernel_size)
>>>>        return get_legal_offset(random, kernel_size_align);
>>>>    }
>>>>
>>>> +void __init kaslr_late_init(void)
>>>> +{
>>>> +     uintptr_t kernel_size;
>>>> +
>>>> +     /* Clear original kernel image. */
>>>> +     if (kaslr_offset) {
>>>> +             kernel_size = (uintptr_t) _end - (uintptr_t) _start;
>>>
>>> kernel_size = (uintptr_t) _end - (uintptr_t) _start + 1;
>>
>> OK, change it in the next version. Thanks.
>>
>>>
>>>> +             memset((void *)PAGE_OFFSET, 0, kernel_size);
>>>
>>> I have been thinking again about our discussion regarding PAGE_OFFSET:
>>> PAGE_OFFSET actually points to the address where the kernel was loaded,
>>> not the beginning of memory, that's a bit weird.
>>>
>>> Just saying that here, because it took me a few seconds to remember that
>>> and understand what you were doing here.
>>
>> In non-kaslr case, we load the kernel to PAGE_OFFSET which points to,
>> so we clear the old kernel image through PAGE_OFFSET here. Certainly,
>> we could use a symbol to record the start address of the old kernel
>> image instead of PAGE_OFFSET here. I don't see other architectures
>> changing PAGE_OFFSET after copying the kernel to the new location in
>> kaslr. If you think the PAGE_OFFSET needs to be changed, we need to
>> give another way to make the page table could create the mappings for
>> the whole memory and memblock/buddy system could see the whole memory
>> after the kernel moves.
>>  >>
>>>> +             set_memory_nx(PAGE_OFFSET, kaslr_offset >> PAGE_SHIFT);
>>>
>>> Again, I certainly missed something but when do you use old kernel
>>> mappings ?
>>
>> We use old kernel mappings when KASLR calculates the random offset, at
>> that moment, kernel is running on old kernel location.
> 
> Yes but haven't you already cleared the page table from the mappings for 
> the old kernel in clear_page_tables called in setup_vm of the new kernel ?
> 
> Alex
> 

I had a doubt so I read set_memory_nx implementation again and I was 
indeed completely wrong: set_memory_nx tackles init_mm and then 
swapper_pg_dir. So you just remove executability for the old kernel 
zone, that's ok I think.

Sorry for the noise !

Alex

>>
>>>
>>>> +     }
>>>> +}
>>>> +
>>>>    uintptr_t __init kaslr_early_init(void)
>>>>    {
>>>>        u64 seed;
>>>> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
>>>> index 34c6ecf2c599..08e2ce170533 100644
>>>> --- a/arch/riscv/mm/init.c
>>>> +++ b/arch/riscv/mm/init.c
>>>> @@ -15,6 +15,7 @@
>>>>    #include <linux/set_memory.h>
>>>>    #ifdef CONFIG_RELOCATABLE
>>>>    #include <linux/elf.h>
>>>> +#include <asm/kaslr.h>
>>>>    #endif
>>>>
>>>>    #include <asm/fixmap.h>
>>>> @@ -649,6 +650,11 @@ static void __init setup_vm_final(void)
>>>>        /* Move to swapper page table */
>>>>        csr_write(CSR_SATP, PFN_DOWN(__pa_symbol(swapper_pg_dir)) | 
>>>> SATP_MODE);
>>>>        local_flush_tlb_all();
>>>> +
>>>> +#ifdef CONFIG_RANDOMIZE_BASE
>>>> +     /* Clear orignial kernel image and set the right permission. */
>>>> +     kaslr_late_init();
>>>> +#endif
>>>>    }
>>>>
>>>>    void free_initmem(void)
>>>>
>>>
>>> Alex
diff mbox series

Patch

diff --git a/arch/riscv/include/asm/kaslr.h b/arch/riscv/include/asm/kaslr.h
new file mode 100644
index 000000000000..b165fe71dd4a
--- /dev/null
+++ b/arch/riscv/include/asm/kaslr.h
@@ -0,0 +1,12 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2020 SiFive
+ * Copyright (C) 2020 Zong Li <zong.li@sifive.com>
+ */
+
+#ifndef _ASM_RISCV_KASLR_H
+#define _ASM_RISCV_KASLR_H
+
+void __init kaslr_late_init(void);
+
+#endif /* _ASM_RISCV_KASLR_H */
diff --git a/arch/riscv/kernel/kaslr.c b/arch/riscv/kernel/kaslr.c
index 59001d6fdfc3..0bd30831c455 100644
--- a/arch/riscv/kernel/kaslr.c
+++ b/arch/riscv/kernel/kaslr.c
@@ -356,6 +356,18 @@  static __init uintptr_t get_random_offset(u64 seed, uintptr_t kernel_size)
 	return get_legal_offset(random, kernel_size_align);
 }
 
+void __init kaslr_late_init(void)
+{
+	uintptr_t kernel_size;
+
+	/* Clear original kernel image. */
+	if (kaslr_offset) {
+		kernel_size = (uintptr_t) _end - (uintptr_t) _start;
+		memset((void *)PAGE_OFFSET, 0, kernel_size);
+		set_memory_nx(PAGE_OFFSET, kaslr_offset >> PAGE_SHIFT);
+	}
+}
+
 uintptr_t __init kaslr_early_init(void)
 {
 	u64 seed;
diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index 34c6ecf2c599..08e2ce170533 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -15,6 +15,7 @@ 
 #include <linux/set_memory.h>
 #ifdef CONFIG_RELOCATABLE
 #include <linux/elf.h>
+#include <asm/kaslr.h>
 #endif
 
 #include <asm/fixmap.h>
@@ -649,6 +650,11 @@  static void __init setup_vm_final(void)
 	/* Move to swapper page table */
 	csr_write(CSR_SATP, PFN_DOWN(__pa_symbol(swapper_pg_dir)) | SATP_MODE);
 	local_flush_tlb_all();
+
+#ifdef CONFIG_RANDOMIZE_BASE
+	/* Clear orignial kernel image and set the right permission. */
+	kaslr_late_init();
+#endif
 }
 
 void free_initmem(void)