Re: [RFC][PATCH] s390, postinit-readonly: implement post-init RO
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.
Re: [RFC][PATCH] s390, postinit-readonly: implement post-init RO
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.
Re: [RFC][PATCH] s390, postinit-readonly: implement post-init RO
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
Re: [RFC][PATCH] s390, postinit-readonly: implement post-init RO
On Mon, Mar 7, 2016 at 4:20 PM, Kees Cook 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
> ---
> 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
>
> -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
