[kernel-hardening] [RFC][PATCH] s390, postinit-readonly: implement post-init RO

[kernel-hardening] [RFC][PATCH] s390, postinit-readonly: implement post-init RO

[Kees Cook]

Since s390 already sets its .rodata section RO from the start, the generic
.data..ro_after_init section is already RO before init runs. For s390,
split the post-init read-only section off separately and handle that
when the call to mark_rodata_ro() is made.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
This is totally untested...
---
 arch/s390/Kconfig                |  3 +++
 arch/s390/include/asm/cache.h    |  2 ++
 arch/s390/include/asm/sections.h |  2 +-
 arch/s390/kernel/vmlinux.lds.S   |  6 ++++++
 arch/s390/mm/init.c              | 10 ++++++++++
 5 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 3be9c832dec1..3f8b96f2cd2d 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -59,6 +59,9 @@ config PCI_QUIRKS
 config ARCH_SUPPORTS_UPROBES
 	def_bool y
 
+config DEBUG_RODATA
+	def_bool y
+
 config S390
 	def_bool y
 	select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
diff --git a/arch/s390/include/asm/cache.h b/arch/s390/include/asm/cache.h
index 4d7ccac5fd1d..816c2964bbee 100644
--- a/arch/s390/include/asm/cache.h
+++ b/arch/s390/include/asm/cache.h
@@ -15,4 +15,6 @@
 
 #define __read_mostly __attribute__((__section__(".data..read_mostly")))
 
+#define __ro_after_init __attribute__((__section__(".arch_ro_after_init")))
+
 #endif
diff --git a/arch/s390/include/asm/sections.h b/arch/s390/include/asm/sections.h
index fbd9116eb17b..6cc6acf87416 100644
--- a/arch/s390/include/asm/sections.h
+++ b/arch/s390/include/asm/sections.h
@@ -3,6 +3,6 @@
 
 #include <asm-generic/sections.h>
 
-extern char _eshared[], _ehead[];
+extern char _eshared[], _ehead[], __ro_after_init[];
 
 #endif
diff --git a/arch/s390/kernel/vmlinux.lds.S b/arch/s390/kernel/vmlinux.lds.S
index 445657fe658c..39a2c7e4cdd2 100644
--- a/arch/s390/kernel/vmlinux.lds.S
+++ b/arch/s390/kernel/vmlinux.lds.S
@@ -52,6 +52,12 @@ SECTIONS
 
 	RW_DATA_SECTION(0x100, PAGE_SIZE, THREAD_SIZE)
 
+	. = ALIGN(PAGE_SIZE)
+	__ro_after_init = .;
+	.arch_ro_after_init : {
+		*(.arch_ro_after_init)	/* Read only after init */
+	}
+
 	_edata = .;		/* End of data section */
 
 	/* will be freed after init */
diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
index 73e290337092..6033d396b96c 100644
--- a/arch/s390/mm/init.c
+++ b/arch/s390/mm/init.c
@@ -136,6 +136,16 @@ void free_initmem(void)
 	free_initmem_default(POISON_FREE_INITMEM);
 }
 
+void mark_rodata_ro(void)
+{
+	unsigned long start = (unsigned long) &__ro_after_init;
+	unsigned long end = (unsigned long) &_edata;
+
+	printk(KERN_INFO "Write protecting post-init read-only data: %luk\n",
+	       (end - start) >> 10);
+	set_memory_ro(start, (end - start) >> PAGE_SHIFT);
+}
+
 #ifdef CONFIG_BLK_DEV_INITRD
 void __init free_initrd_mem(unsigned long start, unsigned long end)
 {
-- 
2.6.3


-- 
Kees Cook
Chrome OS & Brillo Security

[kernel-hardening] Re: [RFC][PATCH] s390, postinit-readonly: implement post-init RO

[Kees Cook]

On Mon, Mar 7, 2016 at 4:20 PM, Kees Cook <keescook@chromium.org> wrote:
> Since s390 already sets its .rodata section RO from the start, the generic
> .data..ro_after_init section is already RO before init runs. For s390,
> split the post-init read-only section off separately and handle that
> when the call to mark_rodata_ro() is made.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> This is totally untested...
> ---
>  arch/s390/Kconfig                |  3 +++
>  arch/s390/include/asm/cache.h    |  2 ++
>  arch/s390/include/asm/sections.h |  2 +-
>  arch/s390/kernel/vmlinux.lds.S   |  6 ++++++
>  arch/s390/mm/init.c              | 10 ++++++++++
>  5 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index 3be9c832dec1..3f8b96f2cd2d 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -59,6 +59,9 @@ config PCI_QUIRKS
>  config ARCH_SUPPORTS_UPROBES
>         def_bool y
>
> +config DEBUG_RODATA
> +       def_bool y
> +
>  config S390
>         def_bool y
>         select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
> diff --git a/arch/s390/include/asm/cache.h b/arch/s390/include/asm/cache.h
> index 4d7ccac5fd1d..816c2964bbee 100644
> --- a/arch/s390/include/asm/cache.h
> +++ b/arch/s390/include/asm/cache.h
> @@ -15,4 +15,6 @@
>
>  #define __read_mostly __attribute__((__section__(".data..read_mostly")))
>
> +#define __ro_after_init __attribute__((__section__(".arch_ro_after_init")))
> +
>  #endif
> diff --git a/arch/s390/include/asm/sections.h b/arch/s390/include/asm/sections.h
> index fbd9116eb17b..6cc6acf87416 100644
> --- a/arch/s390/include/asm/sections.h
> +++ b/arch/s390/include/asm/sections.h
> @@ -3,6 +3,6 @@
>
>  #include <asm-generic/sections.h>
>
> -extern char _eshared[], _ehead[];
> +extern char _eshared[], _ehead[], __ro_after_init[];

0day points out this should be _ro_after_init

>
>  #endif
> diff --git a/arch/s390/kernel/vmlinux.lds.S b/arch/s390/kernel/vmlinux.lds.S
> index 445657fe658c..39a2c7e4cdd2 100644
> --- a/arch/s390/kernel/vmlinux.lds.S
> +++ b/arch/s390/kernel/vmlinux.lds.S
> @@ -52,6 +52,12 @@ SECTIONS
>
>         RW_DATA_SECTION(0x100, PAGE_SIZE, THREAD_SIZE)
>
> +       . = ALIGN(PAGE_SIZE)
> +       __ro_after_init = .;

and here

> +       .arch_ro_after_init : {
> +               *(.arch_ro_after_init)  /* Read only after init */
> +       }
> +
>         _edata = .;             /* End of data section */
>
>         /* will be freed after init */
> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> index 73e290337092..6033d396b96c 100644
> --- a/arch/s390/mm/init.c
> +++ b/arch/s390/mm/init.c
> @@ -136,6 +136,16 @@ void free_initmem(void)
>         free_initmem_default(POISON_FREE_INITMEM);
>  }
>
> +void mark_rodata_ro(void)
> +{
> +       unsigned long start = (unsigned long) &__ro_after_init;

and here. :)

> +       unsigned long end = (unsigned long) &_edata;
> +
> +       printk(KERN_INFO "Write protecting post-init read-only data: %luk\n",
> +              (end - start) >> 10);
> +       set_memory_ro(start, (end - start) >> PAGE_SHIFT);
> +}
> +
>  #ifdef CONFIG_BLK_DEV_INITRD
>  void __init free_initrd_mem(unsigned long start, unsigned long end)
>  {
> --
> 2.6.3
>
>
> --
> Kees Cook
> Chrome OS & Brillo Security

I will go try Andy's virtme next... ;)

-Kees


-- 
Kees Cook
Chrome OS & Brillo Security

[kernel-hardening] Re: [RFC][PATCH] s390, postinit-readonly: implement post-init RO

[Christian Borntraeger]

On 03/08/2016 01:41 AM, Kees Cook wrote:

>> --- a/arch/s390/kernel/vmlinux.lds.S
>> +++ b/arch/s390/kernel/vmlinux.lds.S
>> @@ -52,6 +52,12 @@ SECTIONS
>>
>>         RW_DATA_SECTION(0x100, PAGE_SIZE, THREAD_SIZE)
>>
>> +       . = ALIGN(PAGE_SIZE)


missing ";" ?


With that  and your fixes, this function claims to mark 0kB and 
lkdtm can still write. Reason is that _edata is 0xc11008 and start is
0x0c11000.

making _edata page aligned as well, does now try to mark one page, but then
we run into the next issue, that 

static void change_page_attr(unsigned long addr, int numpages,
                             pte_t (*set) (pte_t))
{
        pte_t *ptep;
        int i;

        for (i = 0; i < numpages; i++) {
                ptep = walk_page_table(addr);

triggers this
                if (WARN_ON_ONCE(!ptep))
                        break;

because the kernel decided to map this with a large page. So we need
to fix this function to then break the large page into a smaller chunk....

Christian

[kernel-hardening] Re: [RFC][PATCH] s390, postinit-readonly: implement post-init RO

[Heiko Carstens]

On Tue, Mar 08, 2016 at 09:51:05AM +0100, Christian Borntraeger wrote:
> On 03/08/2016 01:41 AM, Kees Cook wrote:
> 
> >> --- a/arch/s390/kernel/vmlinux.lds.S
> >> +++ b/arch/s390/kernel/vmlinux.lds.S
> >> @@ -52,6 +52,12 @@ SECTIONS
> >>
> >>         RW_DATA_SECTION(0x100, PAGE_SIZE, THREAD_SIZE)
> >>
> >> +       . = ALIGN(PAGE_SIZE)
> 
> 
> missing ";" ?
> 
> 
> With that  and your fixes, this function claims to mark 0kB and 
> lkdtm can still write. Reason is that _edata is 0xc11008 and start is
> 0x0c11000.
> 
> making _edata page aligned as well, does now try to mark one page, but then
> we run into the next issue, that 
> 
> static void change_page_attr(unsigned long addr, int numpages,
>                              pte_t (*set) (pte_t))
> {
>         pte_t *ptep;
>         int i;
> 
>         for (i = 0; i < numpages; i++) {
>                 ptep = walk_page_table(addr);
> 
> triggers this
>                 if (WARN_ON_ONCE(!ptep))
>                         break;
> 
> because the kernel decided to map this with a large page. So we need
> to fix this function to then break the large page into a smaller chunk....

Yes... however that's a rather large change. I'll try to come up with a
patch that has less impact and implement the code that splits the kernel
mapping later.
Looking at our vmemmap code makes me realize that this code needs also to
be improved.

[kernel-hardening] Re: [RFC][PATCH] s390, postinit-readonly: implement post-init RO

[Heiko Carstens]

On Tue, Mar 08, 2016 at 12:43:15PM +0100, Heiko Carstens wrote:
> On Tue, Mar 08, 2016 at 09:51:05AM +0100, Christian Borntraeger wrote:
> > On 03/08/2016 01:41 AM, Kees Cook wrote:
> > 
> > >> --- a/arch/s390/kernel/vmlinux.lds.S
> > >> +++ b/arch/s390/kernel/vmlinux.lds.S
> > >> @@ -52,6 +52,12 @@ SECTIONS
> > >>
> > >>         RW_DATA_SECTION(0x100, PAGE_SIZE, THREAD_SIZE)
> > >>
> > >> +       . = ALIGN(PAGE_SIZE)
> > 
> > 
> > missing ";" ?
> > 
> > 
> > With that  and your fixes, this function claims to mark 0kB and 
> > lkdtm can still write. Reason is that _edata is 0xc11008 and start is
> > 0x0c11000.
> > 
> > making _edata page aligned as well, does now try to mark one page,

Yes, we do need that fixed.

> > but then
> > we run into the next issue, that 
> > 
> > static void change_page_attr(unsigned long addr, int numpages,
> >                              pte_t (*set) (pte_t))
> > {
> >         pte_t *ptep;
> >         int i;
> > 
> >         for (i = 0; i < numpages; i++) {
> >                 ptep = walk_page_table(addr);
> > 
> > triggers this
> >                 if (WARN_ON_ONCE(!ptep))
> >                         break;
> > 
> > because the kernel decided to map this with a large page. So we need
> > to fix this function to then break the large page into a smaller chunk....
> 
> Yes... however that's a rather large change. I'll try to come up with a
> patch that has less impact and implement the code that splits the kernel
> mapping later.
> Looking at our vmemmap code makes me realize that this code needs also to
> be improved.

Hm, that's going to be too ugly. So we'll go for the "real" solution, which
modifies the kernel mapping instead.