[PATCH] arm64: mm: mark tramp_pg_dir read-only

[PATCH] arm64: mm: mark tramp_pg_dir read-only

[YaoJun]

To protect against KSMA(Kernel Space Mirroring Attack), make
tramp_pg_dir read-only. The principle of KSMA is to insert a
carefully constructed PGD entry into the translation table.
The type of this entry is block, which maps the kernel text
and its access permissions bits are 01. The user process can
then modify kernel text directly through this mapping. In this
way, an arbitrary write can be converted to multiple arbitrary
writes.

Signed-off-by: YaoJun <yaojun8558363@gmail.com>
---
 arch/arm64/mm/mmu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 2dbb2c9f1ec1..ac4b22c7e435 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
 	__create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
 			     prot, pgd_pgtable_alloc, 0);
 
+	update_mapping_prot(__pa_symbol(tramp_pg_dir),
+				(unsigned long)tramp_pg_dir,
+				PGD_SIZE, PAGE_KERNEL_RO);
+
 	/* Map both the text and data into the kernel page table */
 	__set_fixmap(FIX_ENTRY_TRAMP_TEXT, pa_start, prot);
 	if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {
-- 
2.17.0

[PATCH] arm64: mm: mark tramp_pg_dir read-only

[YaoJun]

To protect against KSMA(Kernel Space Mirroring Attack), make
tramp_pg_dir read-only. The principle of KSMA is to insert a
carefully constructed PGD entry into the translation table.
The type of this entry is block, which maps the kernel text
and its access permissions bits are 01. The user process can
then modify kernel text directly through this mapping. In this
way, an arbitrary write can be converted to multiple arbitrary
writes.

Signed-off-by: YaoJun <yaojun8558363@gmail.com>
---
 arch/arm64/mm/mmu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 2dbb2c9f1ec1..ac4b22c7e435 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
 	__create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
 			     prot, pgd_pgtable_alloc, 0);
 
+	update_mapping_prot(__pa_symbol(tramp_pg_dir),
+				(unsigned long)tramp_pg_dir,
+				PGD_SIZE, PAGE_KERNEL_RO);
+
 	/* Map both the text and data into the kernel page table */
 	__set_fixmap(FIX_ENTRY_TRAMP_TEXT, pa_start, prot);
 	if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {
-- 
2.17.0

Re: [PATCH] arm64: mm: mark tramp_pg_dir read-only

[Will Deacon]

On Wed, May 30, 2018 at 12:48:06PM +0800, YaoJun wrote:
> To protect against KSMA(Kernel Space Mirroring Attack), make
> tramp_pg_dir read-only. The principle of KSMA is to insert a
> carefully constructed PGD entry into the translation table.
> The type of this entry is block, which maps the kernel text
> and its access permissions bits are 01. The user process can
> then modify kernel text directly through this mapping. In this
> way, an arbitrary write can be converted to multiple arbitrary
> writes.
> 
> Signed-off-by: YaoJun <yaojun8558363@gmail.com>
> ---
>  arch/arm64/mm/mmu.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 2dbb2c9f1ec1..ac4b22c7e435 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
>  	__create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
>  			     prot, pgd_pgtable_alloc, 0);
>  
> +	update_mapping_prot(__pa_symbol(tramp_pg_dir),
> +				(unsigned long)tramp_pg_dir,
> +				PGD_SIZE, PAGE_KERNEL_RO);

Hmm, I like the idea but is there a risk that the page table has been mapped
as part of a block entry, which we can't safely split at this point (i.e.
we'll run into one of the BUG_ONs in the mapping code)?

Will

[PATCH] arm64: mm: mark tramp_pg_dir read-only

[Will Deacon]

On Wed, May 30, 2018 at 12:48:06PM +0800, YaoJun wrote:
> To protect against KSMA(Kernel Space Mirroring Attack), make
> tramp_pg_dir read-only. The principle of KSMA is to insert a
> carefully constructed PGD entry into the translation table.
> The type of this entry is block, which maps the kernel text
> and its access permissions bits are 01. The user process can
> then modify kernel text directly through this mapping. In this
> way, an arbitrary write can be converted to multiple arbitrary
> writes.
> 
> Signed-off-by: YaoJun <yaojun8558363@gmail.com>
> ---
>  arch/arm64/mm/mmu.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 2dbb2c9f1ec1..ac4b22c7e435 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
>  	__create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
>  			     prot, pgd_pgtable_alloc, 0);
>  
> +	update_mapping_prot(__pa_symbol(tramp_pg_dir),
> +				(unsigned long)tramp_pg_dir,
> +				PGD_SIZE, PAGE_KERNEL_RO);

Hmm, I like the idea but is there a risk that the page table has been mapped
as part of a block entry, which we can't safely split at this point (i.e.
we'll run into one of the BUG_ONs in the mapping code)?

Will

Re: [PATCH] arm64: mm: mark tramp_pg_dir read-only

[Ard Biesheuvel]

On 30 May 2018 at 11:14, Will Deacon <will.deacon@arm.com> wrote:
> On Wed, May 30, 2018 at 12:48:06PM +0800, YaoJun wrote:
>> To protect against KSMA(Kernel Space Mirroring Attack), make
>> tramp_pg_dir read-only. The principle of KSMA is to insert a
>> carefully constructed PGD entry into the translation table.
>> The type of this entry is block, which maps the kernel text
>> and its access permissions bits are 01. The user process can
>> then modify kernel text directly through this mapping. In this
>> way, an arbitrary write can be converted to multiple arbitrary
>> writes.
>>
>> Signed-off-by: YaoJun <yaojun8558363@gmail.com>
>> ---
>>  arch/arm64/mm/mmu.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index 2dbb2c9f1ec1..ac4b22c7e435 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
>>       __create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
>>                            prot, pgd_pgtable_alloc, 0);
>>
>> +     update_mapping_prot(__pa_symbol(tramp_pg_dir),
>> +                             (unsigned long)tramp_pg_dir,
>> +                             PGD_SIZE, PAGE_KERNEL_RO);
>
> Hmm, I like the idea but is there a risk that the page table has been mapped
> as part of a block entry, which we can't safely split at this point (i.e.
> we'll run into one of the BUG_ONs in the mapping code)?
>

We'd need to create a separate segment for it initially so the mapping
is already at the right granularity.

[PATCH] arm64: mm: mark tramp_pg_dir read-only

[Ard Biesheuvel]

On 30 May 2018 at 11:14, Will Deacon <will.deacon@arm.com> wrote:
> On Wed, May 30, 2018 at 12:48:06PM +0800, YaoJun wrote:
>> To protect against KSMA(Kernel Space Mirroring Attack), make
>> tramp_pg_dir read-only. The principle of KSMA is to insert a
>> carefully constructed PGD entry into the translation table.
>> The type of this entry is block, which maps the kernel text
>> and its access permissions bits are 01. The user process can
>> then modify kernel text directly through this mapping. In this
>> way, an arbitrary write can be converted to multiple arbitrary
>> writes.
>>
>> Signed-off-by: YaoJun <yaojun8558363@gmail.com>
>> ---
>>  arch/arm64/mm/mmu.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index 2dbb2c9f1ec1..ac4b22c7e435 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
>>       __create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
>>                            prot, pgd_pgtable_alloc, 0);
>>
>> +     update_mapping_prot(__pa_symbol(tramp_pg_dir),
>> +                             (unsigned long)tramp_pg_dir,
>> +                             PGD_SIZE, PAGE_KERNEL_RO);
>
> Hmm, I like the idea but is there a risk that the page table has been mapped
> as part of a block entry, which we can't safely split at this point (i.e.
> we'll run into one of the BUG_ONs in the mapping code)?
>

We'd need to create a separate segment for it initially so the mapping
is already at the right granularity.

Re: [PATCH] arm64: mm: mark tramp_pg_dir read-only

[姚俊]

[-- Attachment #1: Type: text/plain, Size: 2524 bytes --]

>We'd need to create a separate segment for it initially so the mapping
>is already at the right granularity.

I think that I should to it like your patch, right?

 Subject:    [RFC PATCH 2/6] arm64/mm: create dedicated segment for pgdir
mappings
 From:       Ard Biesheuvel <ard.biesheuvel () linaro ! org>
 Date:       2018-03-19 11:19:54
 Message-ID: 20180319111958.4171-3-ard.biesheuvel () linaro ! org

+       . = ALIGN(SEGMENT_ALIGN);
+       __pgdir_segment_start = .;
#ifdef CONFIG_UNMAP_KERNEL_AT_EL0
         tramp_pg_dir = .;
         . += PAGE_SIZE;
@@ -229,6 +231,7 @@ SECTIONS
         . += RESERVED_TTBR0_SIZE;
#endif
         swapper_pg_dir = .;
 +       __pgdir_segment_end = . + PAGE_SIZE;
         . += SWAPPER_DIR_SIZE;
         swapper_pg_end = .;

Thanks.

Jun


2018-05-30 17:53 GMT+08:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>:

> On 30 May 2018 at 11:14, Will Deacon <will.deacon@arm.com> wrote:
> > On Wed, May 30, 2018 at 12:48:06PM +0800, YaoJun wrote:
> >> To protect against KSMA(Kernel Space Mirroring Attack), make
> >> tramp_pg_dir read-only. The principle of KSMA is to insert a
> >> carefully constructed PGD entry into the translation table.
> >> The type of this entry is block, which maps the kernel text
> >> and its access permissions bits are 01. The user process can
> >> then modify kernel text directly through this mapping. In this
> >> way, an arbitrary write can be converted to multiple arbitrary
> >> writes.
> >>
> >> Signed-off-by: YaoJun <yaojun8558363@gmail.com>
> >> ---
> >>  arch/arm64/mm/mmu.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> >> index 2dbb2c9f1ec1..ac4b22c7e435 100644
> >> --- a/arch/arm64/mm/mmu.c
> >> +++ b/arch/arm64/mm/mmu.c
> >> @@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
> >>       __create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS,
> PAGE_SIZE,
> >>                            prot, pgd_pgtable_alloc, 0);
> >>
> >> +     update_mapping_prot(__pa_symbol(tramp_pg_dir),
> >> +                             (unsigned long)tramp_pg_dir,
> >> +                             PGD_SIZE, PAGE_KERNEL_RO);
> >
> > Hmm, I like the idea but is there a risk that the page table has been
> mapped
> > as part of a block entry, which we can't safely split at this point (i.e.
> > we'll run into one of the BUG_ONs in the mapping code)?
> >
>
> We'd need to create a separate segment for it initially so the mapping
> is already at the right granularity.
>

[-- Attachment #2: Type: text/html, Size: 5270 bytes --]

Re: [PATCH] arm64: mm: mark tramp_pg_dir read-only

[Will Deacon]

Hi Ard,

Sorry, I forgot to reply to this.

On Wed, May 30, 2018 at 11:53:20AM +0200, Ard Biesheuvel wrote:
> On 30 May 2018 at 11:14, Will Deacon <will.deacon@arm.com> wrote:
> > On Wed, May 30, 2018 at 12:48:06PM +0800, YaoJun wrote:
> >> To protect against KSMA(Kernel Space Mirroring Attack), make
> >> tramp_pg_dir read-only. The principle of KSMA is to insert a
> >> carefully constructed PGD entry into the translation table.
> >> The type of this entry is block, which maps the kernel text
> >> and its access permissions bits are 01. The user process can
> >> then modify kernel text directly through this mapping. In this
> >> way, an arbitrary write can be converted to multiple arbitrary
> >> writes.
> >>
> >> Signed-off-by: YaoJun <yaojun8558363@gmail.com>
> >> ---
> >>  arch/arm64/mm/mmu.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> >> index 2dbb2c9f1ec1..ac4b22c7e435 100644
> >> --- a/arch/arm64/mm/mmu.c
> >> +++ b/arch/arm64/mm/mmu.c
> >> @@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
> >>       __create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
> >>                            prot, pgd_pgtable_alloc, 0);
> >>
> >> +     update_mapping_prot(__pa_symbol(tramp_pg_dir),
> >> +                             (unsigned long)tramp_pg_dir,
> >> +                             PGD_SIZE, PAGE_KERNEL_RO);
> >
> > Hmm, I like the idea but is there a risk that the page table has been mapped
> > as part of a block entry, which we can't safely split at this point (i.e.
> > we'll run into one of the BUG_ONs in the mapping code)?
> >
> 
> We'd need to create a separate segment for it initially so the mapping
> is already at the right granularity.

Why do you think that's the case? I can't see anything that guarantees this
for the page table itself.

Will

[PATCH] arm64: mm: mark tramp_pg_dir read-only

[Will Deacon]

Hi Ard,

Sorry, I forgot to reply to this.

On Wed, May 30, 2018 at 11:53:20AM +0200, Ard Biesheuvel wrote:
> On 30 May 2018 at 11:14, Will Deacon <will.deacon@arm.com> wrote:
> > On Wed, May 30, 2018 at 12:48:06PM +0800, YaoJun wrote:
> >> To protect against KSMA(Kernel Space Mirroring Attack), make
> >> tramp_pg_dir read-only. The principle of KSMA is to insert a
> >> carefully constructed PGD entry into the translation table.
> >> The type of this entry is block, which maps the kernel text
> >> and its access permissions bits are 01. The user process can
> >> then modify kernel text directly through this mapping. In this
> >> way, an arbitrary write can be converted to multiple arbitrary
> >> writes.
> >>
> >> Signed-off-by: YaoJun <yaojun8558363@gmail.com>
> >> ---
> >>  arch/arm64/mm/mmu.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> >> index 2dbb2c9f1ec1..ac4b22c7e435 100644
> >> --- a/arch/arm64/mm/mmu.c
> >> +++ b/arch/arm64/mm/mmu.c
> >> @@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
> >>       __create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
> >>                            prot, pgd_pgtable_alloc, 0);
> >>
> >> +     update_mapping_prot(__pa_symbol(tramp_pg_dir),
> >> +                             (unsigned long)tramp_pg_dir,
> >> +                             PGD_SIZE, PAGE_KERNEL_RO);
> >
> > Hmm, I like the idea but is there a risk that the page table has been mapped
> > as part of a block entry, which we can't safely split at this point (i.e.
> > we'll run into one of the BUG_ONs in the mapping code)?
> >
> 
> We'd need to create a separate segment for it initially so the mapping
> is already at the right granularity.

Why do you think that's the case? I can't see anything that guarantees this
for the page table itself.

Will

Re: [PATCH] arm64: mm: mark tramp_pg_dir read-only

[Ard Biesheuvel]

On 19 June 2018 at 17:20, Will Deacon <will.deacon@arm.com> wrote:
> Hi Ard,
>
> Sorry, I forgot to reply to this.
>
> On Wed, May 30, 2018 at 11:53:20AM +0200, Ard Biesheuvel wrote:
>> On 30 May 2018 at 11:14, Will Deacon <will.deacon@arm.com> wrote:
>> > On Wed, May 30, 2018 at 12:48:06PM +0800, YaoJun wrote:
>> >> To protect against KSMA(Kernel Space Mirroring Attack), make
>> >> tramp_pg_dir read-only. The principle of KSMA is to insert a
>> >> carefully constructed PGD entry into the translation table.
>> >> The type of this entry is block, which maps the kernel text
>> >> and its access permissions bits are 01. The user process can
>> >> then modify kernel text directly through this mapping. In this
>> >> way, an arbitrary write can be converted to multiple arbitrary
>> >> writes.
>> >>
>> >> Signed-off-by: YaoJun <yaojun8558363@gmail.com>
>> >> ---
>> >>  arch/arm64/mm/mmu.c | 4 ++++
>> >>  1 file changed, 4 insertions(+)
>> >>
>> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> >> index 2dbb2c9f1ec1..ac4b22c7e435 100644
>> >> --- a/arch/arm64/mm/mmu.c
>> >> +++ b/arch/arm64/mm/mmu.c
>> >> @@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
>> >>       __create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
>> >>                            prot, pgd_pgtable_alloc, 0);
>> >>
>> >> +     update_mapping_prot(__pa_symbol(tramp_pg_dir),
>> >> +                             (unsigned long)tramp_pg_dir,
>> >> +                             PGD_SIZE, PAGE_KERNEL_RO);
>> >
>> > Hmm, I like the idea but is there a risk that the page table has been mapped
>> > as part of a block entry, which we can't safely split at this point (i.e.
>> > we'll run into one of the BUG_ONs in the mapping code)?
>> >
>>
>> We'd need to create a separate segment for it initially so the mapping
>> is already at the right granularity.
>
> Why do you think that's the case? I can't see anything that guarantees this
> for the page table itself.
>

We'd need to pass NO_BLOCK_MAPPINGS to map_kernel_segment(),
obviously, but that shouldn't hurt since that segment is relatively
tiny anyway.

[PATCH] arm64: mm: mark tramp_pg_dir read-only

[Ard Biesheuvel]

On 19 June 2018 at 17:20, Will Deacon <will.deacon@arm.com> wrote:
> Hi Ard,
>
> Sorry, I forgot to reply to this.
>
> On Wed, May 30, 2018 at 11:53:20AM +0200, Ard Biesheuvel wrote:
>> On 30 May 2018 at 11:14, Will Deacon <will.deacon@arm.com> wrote:
>> > On Wed, May 30, 2018 at 12:48:06PM +0800, YaoJun wrote:
>> >> To protect against KSMA(Kernel Space Mirroring Attack), make
>> >> tramp_pg_dir read-only. The principle of KSMA is to insert a
>> >> carefully constructed PGD entry into the translation table.
>> >> The type of this entry is block, which maps the kernel text
>> >> and its access permissions bits are 01. The user process can
>> >> then modify kernel text directly through this mapping. In this
>> >> way, an arbitrary write can be converted to multiple arbitrary
>> >> writes.
>> >>
>> >> Signed-off-by: YaoJun <yaojun8558363@gmail.com>
>> >> ---
>> >>  arch/arm64/mm/mmu.c | 4 ++++
>> >>  1 file changed, 4 insertions(+)
>> >>
>> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> >> index 2dbb2c9f1ec1..ac4b22c7e435 100644
>> >> --- a/arch/arm64/mm/mmu.c
>> >> +++ b/arch/arm64/mm/mmu.c
>> >> @@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
>> >>       __create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
>> >>                            prot, pgd_pgtable_alloc, 0);
>> >>
>> >> +     update_mapping_prot(__pa_symbol(tramp_pg_dir),
>> >> +                             (unsigned long)tramp_pg_dir,
>> >> +                             PGD_SIZE, PAGE_KERNEL_RO);
>> >
>> > Hmm, I like the idea but is there a risk that the page table has been mapped
>> > as part of a block entry, which we can't safely split at this point (i.e.
>> > we'll run into one of the BUG_ONs in the mapping code)?
>> >
>>
>> We'd need to create a separate segment for it initially so the mapping
>> is already at the right granularity.
>
> Why do you think that's the case? I can't see anything that guarantees this
> for the page table itself.
>

We'd need to pass NO_BLOCK_MAPPINGS to map_kernel_segment(),
obviously, but that shouldn't hurt since that segment is relatively
tiny anyway.

Re: [PATCH] arm64: mm: mark tramp_pg_dir read-only

[Will Deacon]

On Tue, Jun 19, 2018 at 05:23:41PM +0200, Ard Biesheuvel wrote:
> On 19 June 2018 at 17:20, Will Deacon <will.deacon@arm.com> wrote:
> > Hi Ard,
> >
> > Sorry, I forgot to reply to this.
> >
> > On Wed, May 30, 2018 at 11:53:20AM +0200, Ard Biesheuvel wrote:
> >> On 30 May 2018 at 11:14, Will Deacon <will.deacon@arm.com> wrote:
> >> > On Wed, May 30, 2018 at 12:48:06PM +0800, YaoJun wrote:
> >> >> To protect against KSMA(Kernel Space Mirroring Attack), make
> >> >> tramp_pg_dir read-only. The principle of KSMA is to insert a
> >> >> carefully constructed PGD entry into the translation table.
> >> >> The type of this entry is block, which maps the kernel text
> >> >> and its access permissions bits are 01. The user process can
> >> >> then modify kernel text directly through this mapping. In this
> >> >> way, an arbitrary write can be converted to multiple arbitrary
> >> >> writes.
> >> >>
> >> >> Signed-off-by: YaoJun <yaojun8558363@gmail.com>
> >> >> ---
> >> >>  arch/arm64/mm/mmu.c | 4 ++++
> >> >>  1 file changed, 4 insertions(+)
> >> >>
> >> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> >> >> index 2dbb2c9f1ec1..ac4b22c7e435 100644
> >> >> --- a/arch/arm64/mm/mmu.c
> >> >> +++ b/arch/arm64/mm/mmu.c
> >> >> @@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
> >> >>       __create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
> >> >>                            prot, pgd_pgtable_alloc, 0);
> >> >>
> >> >> +     update_mapping_prot(__pa_symbol(tramp_pg_dir),
> >> >> +                             (unsigned long)tramp_pg_dir,
> >> >> +                             PGD_SIZE, PAGE_KERNEL_RO);
> >> >
> >> > Hmm, I like the idea but is there a risk that the page table has been mapped
> >> > as part of a block entry, which we can't safely split at this point (i.e.
> >> > we'll run into one of the BUG_ONs in the mapping code)?
> >> >
> >>
> >> We'd need to create a separate segment for it initially so the mapping
> >> is already at the right granularity.
> >
> > Why do you think that's the case? I can't see anything that guarantees this
> > for the page table itself.
> >
> 
> We'd need to pass NO_BLOCK_MAPPINGS to map_kernel_segment(),
> obviously, but that shouldn't hurt since that segment is relatively
> tiny anyway.

Ah right, with NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS, I agree that we're good.
Ideally, we'd move {idmap,swapper,tramp}_pg_dir into .rodata...

Will

[PATCH] arm64: mm: mark tramp_pg_dir read-only

[Will Deacon]

On Tue, Jun 19, 2018 at 05:23:41PM +0200, Ard Biesheuvel wrote:
> On 19 June 2018 at 17:20, Will Deacon <will.deacon@arm.com> wrote:
> > Hi Ard,
> >
> > Sorry, I forgot to reply to this.
> >
> > On Wed, May 30, 2018 at 11:53:20AM +0200, Ard Biesheuvel wrote:
> >> On 30 May 2018 at 11:14, Will Deacon <will.deacon@arm.com> wrote:
> >> > On Wed, May 30, 2018 at 12:48:06PM +0800, YaoJun wrote:
> >> >> To protect against KSMA(Kernel Space Mirroring Attack), make
> >> >> tramp_pg_dir read-only. The principle of KSMA is to insert a
> >> >> carefully constructed PGD entry into the translation table.
> >> >> The type of this entry is block, which maps the kernel text
> >> >> and its access permissions bits are 01. The user process can
> >> >> then modify kernel text directly through this mapping. In this
> >> >> way, an arbitrary write can be converted to multiple arbitrary
> >> >> writes.
> >> >>
> >> >> Signed-off-by: YaoJun <yaojun8558363@gmail.com>
> >> >> ---
> >> >>  arch/arm64/mm/mmu.c | 4 ++++
> >> >>  1 file changed, 4 insertions(+)
> >> >>
> >> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> >> >> index 2dbb2c9f1ec1..ac4b22c7e435 100644
> >> >> --- a/arch/arm64/mm/mmu.c
> >> >> +++ b/arch/arm64/mm/mmu.c
> >> >> @@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
> >> >>       __create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
> >> >>                            prot, pgd_pgtable_alloc, 0);
> >> >>
> >> >> +     update_mapping_prot(__pa_symbol(tramp_pg_dir),
> >> >> +                             (unsigned long)tramp_pg_dir,
> >> >> +                             PGD_SIZE, PAGE_KERNEL_RO);
> >> >
> >> > Hmm, I like the idea but is there a risk that the page table has been mapped
> >> > as part of a block entry, which we can't safely split at this point (i.e.
> >> > we'll run into one of the BUG_ONs in the mapping code)?
> >> >
> >>
> >> We'd need to create a separate segment for it initially so the mapping
> >> is already at the right granularity.
> >
> > Why do you think that's the case? I can't see anything that guarantees this
> > for the page table itself.
> >
> 
> We'd need to pass NO_BLOCK_MAPPINGS to map_kernel_segment(),
> obviously, but that shouldn't hurt since that segment is relatively
> tiny anyway.

Ah right, with NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS, I agree that we're good.
Ideally, we'd move {idmap,swapper,tramp}_pg_dir into .rodata...

Will

Re: [PATCH] arm64: mm: mark tramp_pg_dir read-only

[Ard Biesheuvel]

On 19 June 2018 at 17:28, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Jun 19, 2018 at 05:23:41PM +0200, Ard Biesheuvel wrote:
>> On 19 June 2018 at 17:20, Will Deacon <will.deacon@arm.com> wrote:
>> > Hi Ard,
>> >
>> > Sorry, I forgot to reply to this.
>> >
>> > On Wed, May 30, 2018 at 11:53:20AM +0200, Ard Biesheuvel wrote:
>> >> On 30 May 2018 at 11:14, Will Deacon <will.deacon@arm.com> wrote:
>> >> > On Wed, May 30, 2018 at 12:48:06PM +0800, YaoJun wrote:
>> >> >> To protect against KSMA(Kernel Space Mirroring Attack), make
>> >> >> tramp_pg_dir read-only. The principle of KSMA is to insert a
>> >> >> carefully constructed PGD entry into the translation table.
>> >> >> The type of this entry is block, which maps the kernel text
>> >> >> and its access permissions bits are 01. The user process can
>> >> >> then modify kernel text directly through this mapping. In this
>> >> >> way, an arbitrary write can be converted to multiple arbitrary
>> >> >> writes.
>> >> >>
>> >> >> Signed-off-by: YaoJun <yaojun8558363@gmail.com>
>> >> >> ---
>> >> >>  arch/arm64/mm/mmu.c | 4 ++++
>> >> >>  1 file changed, 4 insertions(+)
>> >> >>
>> >> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> >> >> index 2dbb2c9f1ec1..ac4b22c7e435 100644
>> >> >> --- a/arch/arm64/mm/mmu.c
>> >> >> +++ b/arch/arm64/mm/mmu.c
>> >> >> @@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
>> >> >>       __create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
>> >> >>                            prot, pgd_pgtable_alloc, 0);
>> >> >>
>> >> >> +     update_mapping_prot(__pa_symbol(tramp_pg_dir),
>> >> >> +                             (unsigned long)tramp_pg_dir,
>> >> >> +                             PGD_SIZE, PAGE_KERNEL_RO);
>> >> >
>> >> > Hmm, I like the idea but is there a risk that the page table has been mapped
>> >> > as part of a block entry, which we can't safely split at this point (i.e.
>> >> > we'll run into one of the BUG_ONs in the mapping code)?
>> >> >
>> >>
>> >> We'd need to create a separate segment for it initially so the mapping
>> >> is already at the right granularity.
>> >
>> > Why do you think that's the case? I can't see anything that guarantees this
>> > for the page table itself.
>> >
>>
>> We'd need to pass NO_BLOCK_MAPPINGS to map_kernel_segment(),
>> obviously, but that shouldn't hurt since that segment is relatively
>> tiny anyway.
>
> Ah right, with NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS, I agree that we're good.
> Ideally, we'd move {idmap,swapper,tramp}_pg_dir into .rodata...
>

idmap and tramp yes, but swapper needs to be modifiable at runtime, no?

[PATCH] arm64: mm: mark tramp_pg_dir read-only

[Ard Biesheuvel]

On 19 June 2018 at 17:28, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Jun 19, 2018 at 05:23:41PM +0200, Ard Biesheuvel wrote:
>> On 19 June 2018 at 17:20, Will Deacon <will.deacon@arm.com> wrote:
>> > Hi Ard,
>> >
>> > Sorry, I forgot to reply to this.
>> >
>> > On Wed, May 30, 2018 at 11:53:20AM +0200, Ard Biesheuvel wrote:
>> >> On 30 May 2018 at 11:14, Will Deacon <will.deacon@arm.com> wrote:
>> >> > On Wed, May 30, 2018 at 12:48:06PM +0800, YaoJun wrote:
>> >> >> To protect against KSMA(Kernel Space Mirroring Attack), make
>> >> >> tramp_pg_dir read-only. The principle of KSMA is to insert a
>> >> >> carefully constructed PGD entry into the translation table.
>> >> >> The type of this entry is block, which maps the kernel text
>> >> >> and its access permissions bits are 01. The user process can
>> >> >> then modify kernel text directly through this mapping. In this
>> >> >> way, an arbitrary write can be converted to multiple arbitrary
>> >> >> writes.
>> >> >>
>> >> >> Signed-off-by: YaoJun <yaojun8558363@gmail.com>
>> >> >> ---
>> >> >>  arch/arm64/mm/mmu.c | 4 ++++
>> >> >>  1 file changed, 4 insertions(+)
>> >> >>
>> >> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> >> >> index 2dbb2c9f1ec1..ac4b22c7e435 100644
>> >> >> --- a/arch/arm64/mm/mmu.c
>> >> >> +++ b/arch/arm64/mm/mmu.c
>> >> >> @@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
>> >> >>       __create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
>> >> >>                            prot, pgd_pgtable_alloc, 0);
>> >> >>
>> >> >> +     update_mapping_prot(__pa_symbol(tramp_pg_dir),
>> >> >> +                             (unsigned long)tramp_pg_dir,
>> >> >> +                             PGD_SIZE, PAGE_KERNEL_RO);
>> >> >
>> >> > Hmm, I like the idea but is there a risk that the page table has been mapped
>> >> > as part of a block entry, which we can't safely split at this point (i.e.
>> >> > we'll run into one of the BUG_ONs in the mapping code)?
>> >> >
>> >>
>> >> We'd need to create a separate segment for it initially so the mapping
>> >> is already at the right granularity.
>> >
>> > Why do you think that's the case? I can't see anything that guarantees this
>> > for the page table itself.
>> >
>>
>> We'd need to pass NO_BLOCK_MAPPINGS to map_kernel_segment(),
>> obviously, but that shouldn't hurt since that segment is relatively
>> tiny anyway.
>
> Ah right, with NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS, I agree that we're good.
> Ideally, we'd move {idmap,swapper,tramp}_pg_dir into .rodata...
>

idmap and tramp yes, but swapper needs to be modifiable at runtime, no?

Re: [PATCH] arm64: mm: mark tramp_pg_dir read-only

[Will Deacon]

On Tue, Jun 19, 2018 at 05:29:03PM +0200, Ard Biesheuvel wrote:
> On 19 June 2018 at 17:28, Will Deacon <will.deacon@arm.com> wrote:
> > On Tue, Jun 19, 2018 at 05:23:41PM +0200, Ard Biesheuvel wrote:
> >> On 19 June 2018 at 17:20, Will Deacon <will.deacon@arm.com> wrote:
> >> > Hi Ard,
> >> >
> >> > Sorry, I forgot to reply to this.
> >> >
> >> > On Wed, May 30, 2018 at 11:53:20AM +0200, Ard Biesheuvel wrote:
> >> >> On 30 May 2018 at 11:14, Will Deacon <will.deacon@arm.com> wrote:
> >> >> > On Wed, May 30, 2018 at 12:48:06PM +0800, YaoJun wrote:
> >> >> >> To protect against KSMA(Kernel Space Mirroring Attack), make
> >> >> >> tramp_pg_dir read-only. The principle of KSMA is to insert a
> >> >> >> carefully constructed PGD entry into the translation table.
> >> >> >> The type of this entry is block, which maps the kernel text
> >> >> >> and its access permissions bits are 01. The user process can
> >> >> >> then modify kernel text directly through this mapping. In this
> >> >> >> way, an arbitrary write can be converted to multiple arbitrary
> >> >> >> writes.
> >> >> >>
> >> >> >> Signed-off-by: YaoJun <yaojun8558363@gmail.com>
> >> >> >> ---
> >> >> >>  arch/arm64/mm/mmu.c | 4 ++++
> >> >> >>  1 file changed, 4 insertions(+)
> >> >> >>
> >> >> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> >> >> >> index 2dbb2c9f1ec1..ac4b22c7e435 100644
> >> >> >> --- a/arch/arm64/mm/mmu.c
> >> >> >> +++ b/arch/arm64/mm/mmu.c
> >> >> >> @@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
> >> >> >>       __create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
> >> >> >>                            prot, pgd_pgtable_alloc, 0);
> >> >> >>
> >> >> >> +     update_mapping_prot(__pa_symbol(tramp_pg_dir),
> >> >> >> +                             (unsigned long)tramp_pg_dir,
> >> >> >> +                             PGD_SIZE, PAGE_KERNEL_RO);
> >> >> >
> >> >> > Hmm, I like the idea but is there a risk that the page table has been mapped
> >> >> > as part of a block entry, which we can't safely split at this point (i.e.
> >> >> > we'll run into one of the BUG_ONs in the mapping code)?
> >> >> >
> >> >>
> >> >> We'd need to create a separate segment for it initially so the mapping
> >> >> is already at the right granularity.
> >> >
> >> > Why do you think that's the case? I can't see anything that guarantees this
> >> > for the page table itself.
> >> >
> >>
> >> We'd need to pass NO_BLOCK_MAPPINGS to map_kernel_segment(),
> >> obviously, but that shouldn't hurt since that segment is relatively
> >> tiny anyway.
> >
> > Ah right, with NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS, I agree that we're good.
> > Ideally, we'd move {idmap,swapper,tramp}_pg_dir into .rodata...
> >
> 
> idmap and tramp yes, but swapper needs to be modifiable at runtime, no?

Right, but couldn't we swizzle the permissions in e.g. set_pmd? We could
even predicate that on a sanity check of the prot.

Will

[PATCH] arm64: mm: mark tramp_pg_dir read-only

[Will Deacon]

On Tue, Jun 19, 2018 at 05:29:03PM +0200, Ard Biesheuvel wrote:
> On 19 June 2018 at 17:28, Will Deacon <will.deacon@arm.com> wrote:
> > On Tue, Jun 19, 2018 at 05:23:41PM +0200, Ard Biesheuvel wrote:
> >> On 19 June 2018 at 17:20, Will Deacon <will.deacon@arm.com> wrote:
> >> > Hi Ard,
> >> >
> >> > Sorry, I forgot to reply to this.
> >> >
> >> > On Wed, May 30, 2018 at 11:53:20AM +0200, Ard Biesheuvel wrote:
> >> >> On 30 May 2018 at 11:14, Will Deacon <will.deacon@arm.com> wrote:
> >> >> > On Wed, May 30, 2018 at 12:48:06PM +0800, YaoJun wrote:
> >> >> >> To protect against KSMA(Kernel Space Mirroring Attack), make
> >> >> >> tramp_pg_dir read-only. The principle of KSMA is to insert a
> >> >> >> carefully constructed PGD entry into the translation table.
> >> >> >> The type of this entry is block, which maps the kernel text
> >> >> >> and its access permissions bits are 01. The user process can
> >> >> >> then modify kernel text directly through this mapping. In this
> >> >> >> way, an arbitrary write can be converted to multiple arbitrary
> >> >> >> writes.
> >> >> >>
> >> >> >> Signed-off-by: YaoJun <yaojun8558363@gmail.com>
> >> >> >> ---
> >> >> >>  arch/arm64/mm/mmu.c | 4 ++++
> >> >> >>  1 file changed, 4 insertions(+)
> >> >> >>
> >> >> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> >> >> >> index 2dbb2c9f1ec1..ac4b22c7e435 100644
> >> >> >> --- a/arch/arm64/mm/mmu.c
> >> >> >> +++ b/arch/arm64/mm/mmu.c
> >> >> >> @@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
> >> >> >>       __create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
> >> >> >>                            prot, pgd_pgtable_alloc, 0);
> >> >> >>
> >> >> >> +     update_mapping_prot(__pa_symbol(tramp_pg_dir),
> >> >> >> +                             (unsigned long)tramp_pg_dir,
> >> >> >> +                             PGD_SIZE, PAGE_KERNEL_RO);
> >> >> >
> >> >> > Hmm, I like the idea but is there a risk that the page table has been mapped
> >> >> > as part of a block entry, which we can't safely split at this point (i.e.
> >> >> > we'll run into one of the BUG_ONs in the mapping code)?
> >> >> >
> >> >>
> >> >> We'd need to create a separate segment for it initially so the mapping
> >> >> is already at the right granularity.
> >> >
> >> > Why do you think that's the case? I can't see anything that guarantees this
> >> > for the page table itself.
> >> >
> >>
> >> We'd need to pass NO_BLOCK_MAPPINGS to map_kernel_segment(),
> >> obviously, but that shouldn't hurt since that segment is relatively
> >> tiny anyway.
> >
> > Ah right, with NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS, I agree that we're good.
> > Ideally, we'd move {idmap,swapper,tramp}_pg_dir into .rodata...
> >
> 
> idmap and tramp yes, but swapper needs to be modifiable at runtime, no?

Right, but couldn't we swizzle the permissions in e.g. set_pmd? We could
even predicate that on a sanity check of the prot.

Will

Re: [PATCH] arm64: mm: mark tramp_pg_dir read-only

[Ard Biesheuvel]

On 19 June 2018 at 17:37, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Jun 19, 2018 at 05:29:03PM +0200, Ard Biesheuvel wrote:
>> On 19 June 2018 at 17:28, Will Deacon <will.deacon@arm.com> wrote:
>> > On Tue, Jun 19, 2018 at 05:23:41PM +0200, Ard Biesheuvel wrote:
>> >> On 19 June 2018 at 17:20, Will Deacon <will.deacon@arm.com> wrote:
>> >> > Hi Ard,
>> >> >
>> >> > Sorry, I forgot to reply to this.
>> >> >
>> >> > On Wed, May 30, 2018 at 11:53:20AM +0200, Ard Biesheuvel wrote:
>> >> >> On 30 May 2018 at 11:14, Will Deacon <will.deacon@arm.com> wrote:
>> >> >> > On Wed, May 30, 2018 at 12:48:06PM +0800, YaoJun wrote:
>> >> >> >> To protect against KSMA(Kernel Space Mirroring Attack), make
>> >> >> >> tramp_pg_dir read-only. The principle of KSMA is to insert a
>> >> >> >> carefully constructed PGD entry into the translation table.
>> >> >> >> The type of this entry is block, which maps the kernel text
>> >> >> >> and its access permissions bits are 01. The user process can
>> >> >> >> then modify kernel text directly through this mapping. In this
>> >> >> >> way, an arbitrary write can be converted to multiple arbitrary
>> >> >> >> writes.
>> >> >> >>
>> >> >> >> Signed-off-by: YaoJun <yaojun8558363@gmail.com>
>> >> >> >> ---
>> >> >> >>  arch/arm64/mm/mmu.c | 4 ++++
>> >> >> >>  1 file changed, 4 insertions(+)
>> >> >> >>
>> >> >> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> >> >> >> index 2dbb2c9f1ec1..ac4b22c7e435 100644
>> >> >> >> --- a/arch/arm64/mm/mmu.c
>> >> >> >> +++ b/arch/arm64/mm/mmu.c
>> >> >> >> @@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
>> >> >> >>       __create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
>> >> >> >>                            prot, pgd_pgtable_alloc, 0);
>> >> >> >>
>> >> >> >> +     update_mapping_prot(__pa_symbol(tramp_pg_dir),
>> >> >> >> +                             (unsigned long)tramp_pg_dir,
>> >> >> >> +                             PGD_SIZE, PAGE_KERNEL_RO);
>> >> >> >
>> >> >> > Hmm, I like the idea but is there a risk that the page table has been mapped
>> >> >> > as part of a block entry, which we can't safely split at this point (i.e.
>> >> >> > we'll run into one of the BUG_ONs in the mapping code)?
>> >> >> >
>> >> >>
>> >> >> We'd need to create a separate segment for it initially so the mapping
>> >> >> is already at the right granularity.
>> >> >
>> >> > Why do you think that's the case? I can't see anything that guarantees this
>> >> > for the page table itself.
>> >> >
>> >>
>> >> We'd need to pass NO_BLOCK_MAPPINGS to map_kernel_segment(),
>> >> obviously, but that shouldn't hurt since that segment is relatively
>> >> tiny anyway.
>> >
>> > Ah right, with NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS, I agree that we're good.
>> > Ideally, we'd move {idmap,swapper,tramp}_pg_dir into .rodata...
>> >
>>
>> idmap and tramp yes, but swapper needs to be modifiable at runtime, no?
>
> Right, but couldn't we swizzle the permissions in e.g. set_pmd? We could
> even predicate that on a sanity check of the prot.
>

Swizzle the permissions of the entire .rodata segment? That sounds
doable, but there is a whole class of data that belongs in this
category, and I think PaX/grsecurity had an API for that (but I don't
think anyone is upstreaming that yet). So let's not reinvent that
wheel for swapper_pg_dir only.

[PATCH] arm64: mm: mark tramp_pg_dir read-only

[Ard Biesheuvel]

On 19 June 2018 at 17:37, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Jun 19, 2018 at 05:29:03PM +0200, Ard Biesheuvel wrote:
>> On 19 June 2018 at 17:28, Will Deacon <will.deacon@arm.com> wrote:
>> > On Tue, Jun 19, 2018 at 05:23:41PM +0200, Ard Biesheuvel wrote:
>> >> On 19 June 2018 at 17:20, Will Deacon <will.deacon@arm.com> wrote:
>> >> > Hi Ard,
>> >> >
>> >> > Sorry, I forgot to reply to this.
>> >> >
>> >> > On Wed, May 30, 2018 at 11:53:20AM +0200, Ard Biesheuvel wrote:
>> >> >> On 30 May 2018 at 11:14, Will Deacon <will.deacon@arm.com> wrote:
>> >> >> > On Wed, May 30, 2018 at 12:48:06PM +0800, YaoJun wrote:
>> >> >> >> To protect against KSMA(Kernel Space Mirroring Attack), make
>> >> >> >> tramp_pg_dir read-only. The principle of KSMA is to insert a
>> >> >> >> carefully constructed PGD entry into the translation table.
>> >> >> >> The type of this entry is block, which maps the kernel text
>> >> >> >> and its access permissions bits are 01. The user process can
>> >> >> >> then modify kernel text directly through this mapping. In this
>> >> >> >> way, an arbitrary write can be converted to multiple arbitrary
>> >> >> >> writes.
>> >> >> >>
>> >> >> >> Signed-off-by: YaoJun <yaojun8558363@gmail.com>
>> >> >> >> ---
>> >> >> >>  arch/arm64/mm/mmu.c | 4 ++++
>> >> >> >>  1 file changed, 4 insertions(+)
>> >> >> >>
>> >> >> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> >> >> >> index 2dbb2c9f1ec1..ac4b22c7e435 100644
>> >> >> >> --- a/arch/arm64/mm/mmu.c
>> >> >> >> +++ b/arch/arm64/mm/mmu.c
>> >> >> >> @@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
>> >> >> >>       __create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
>> >> >> >>                            prot, pgd_pgtable_alloc, 0);
>> >> >> >>
>> >> >> >> +     update_mapping_prot(__pa_symbol(tramp_pg_dir),
>> >> >> >> +                             (unsigned long)tramp_pg_dir,
>> >> >> >> +                             PGD_SIZE, PAGE_KERNEL_RO);
>> >> >> >
>> >> >> > Hmm, I like the idea but is there a risk that the page table has been mapped
>> >> >> > as part of a block entry, which we can't safely split at this point (i.e.
>> >> >> > we'll run into one of the BUG_ONs in the mapping code)?
>> >> >> >
>> >> >>
>> >> >> We'd need to create a separate segment for it initially so the mapping
>> >> >> is already at the right granularity.
>> >> >
>> >> > Why do you think that's the case? I can't see anything that guarantees this
>> >> > for the page table itself.
>> >> >
>> >>
>> >> We'd need to pass NO_BLOCK_MAPPINGS to map_kernel_segment(),
>> >> obviously, but that shouldn't hurt since that segment is relatively
>> >> tiny anyway.
>> >
>> > Ah right, with NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS, I agree that we're good.
>> > Ideally, we'd move {idmap,swapper,tramp}_pg_dir into .rodata...
>> >
>>
>> idmap and tramp yes, but swapper needs to be modifiable at runtime, no?
>
> Right, but couldn't we swizzle the permissions in e.g. set_pmd? We could
> even predicate that on a sanity check of the prot.
>

Swizzle the permissions of the entire .rodata segment? That sounds
doable, but there is a whole class of data that belongs in this
category, and I think PaX/grsecurity had an API for that (but I don't
think anyone is upstreaming that yet). So let's not reinvent that
wheel for swapper_pg_dir only.

Re: [PATCH] arm64: mm: mark tramp_pg_dir read-only

[Will Deacon]

On Tue, Jun 19, 2018 at 05:40:26PM +0200, Ard Biesheuvel wrote:
> On 19 June 2018 at 17:37, Will Deacon <will.deacon@arm.com> wrote:
> > On Tue, Jun 19, 2018 at 05:29:03PM +0200, Ard Biesheuvel wrote:
> >> On 19 June 2018 at 17:28, Will Deacon <will.deacon@arm.com> wrote:
> >> > On Tue, Jun 19, 2018 at 05:23:41PM +0200, Ard Biesheuvel wrote:
> >> >> On 19 June 2018 at 17:20, Will Deacon <will.deacon@arm.com> wrote:
> >> >> > Hi Ard,
> >> >> >
> >> >> > Sorry, I forgot to reply to this.
> >> >> >
> >> >> > On Wed, May 30, 2018 at 11:53:20AM +0200, Ard Biesheuvel wrote:
> >> >> >> On 30 May 2018 at 11:14, Will Deacon <will.deacon@arm.com> wrote:
> >> >> >> > On Wed, May 30, 2018 at 12:48:06PM +0800, YaoJun wrote:
> >> >> >> >> To protect against KSMA(Kernel Space Mirroring Attack), make
> >> >> >> >> tramp_pg_dir read-only. The principle of KSMA is to insert a
> >> >> >> >> carefully constructed PGD entry into the translation table.
> >> >> >> >> The type of this entry is block, which maps the kernel text
> >> >> >> >> and its access permissions bits are 01. The user process can
> >> >> >> >> then modify kernel text directly through this mapping. In this
> >> >> >> >> way, an arbitrary write can be converted to multiple arbitrary
> >> >> >> >> writes.
> >> >> >> >>
> >> >> >> >> Signed-off-by: YaoJun <yaojun8558363@gmail.com>
> >> >> >> >> ---
> >> >> >> >>  arch/arm64/mm/mmu.c | 4 ++++
> >> >> >> >>  1 file changed, 4 insertions(+)
> >> >> >> >>
> >> >> >> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> >> >> >> >> index 2dbb2c9f1ec1..ac4b22c7e435 100644
> >> >> >> >> --- a/arch/arm64/mm/mmu.c
> >> >> >> >> +++ b/arch/arm64/mm/mmu.c
> >> >> >> >> @@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
> >> >> >> >>       __create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
> >> >> >> >>                            prot, pgd_pgtable_alloc, 0);
> >> >> >> >>
> >> >> >> >> +     update_mapping_prot(__pa_symbol(tramp_pg_dir),
> >> >> >> >> +                             (unsigned long)tramp_pg_dir,
> >> >> >> >> +                             PGD_SIZE, PAGE_KERNEL_RO);
> >> >> >> >
> >> >> >> > Hmm, I like the idea but is there a risk that the page table has been mapped
> >> >> >> > as part of a block entry, which we can't safely split at this point (i.e.
> >> >> >> > we'll run into one of the BUG_ONs in the mapping code)?
> >> >> >> >
> >> >> >>
> >> >> >> We'd need to create a separate segment for it initially so the mapping
> >> >> >> is already at the right granularity.
> >> >> >
> >> >> > Why do you think that's the case? I can't see anything that guarantees this
> >> >> > for the page table itself.
> >> >> >
> >> >>
> >> >> We'd need to pass NO_BLOCK_MAPPINGS to map_kernel_segment(),
> >> >> obviously, but that shouldn't hurt since that segment is relatively
> >> >> tiny anyway.
> >> >
> >> > Ah right, with NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS, I agree that we're good.
> >> > Ideally, we'd move {idmap,swapper,tramp}_pg_dir into .rodata...
> >> >
> >>
> >> idmap and tramp yes, but swapper needs to be modifiable at runtime, no?
> >
> > Right, but couldn't we swizzle the permissions in e.g. set_pmd? We could
> > even predicate that on a sanity check of the prot.
> >
> 
> Swizzle the permissions of the entire .rodata segment? That sounds
> doable, but there is a whole class of data that belongs in this
> category, and I think PaX/grsecurity had an API for that (but I don't
> think anyone is upstreaming that yet). So let's not reinvent that
> wheel for swapper_pg_dir only.

I wasn't thinking of the whole .rodata segment -- just the page containing
the entry being modified, but ok. I was hoping to avoid getting involved
with the PaX/grsec stuff as it seems rather toxic from what I've seen.

Will

[PATCH] arm64: mm: mark tramp_pg_dir read-only

[Will Deacon]

On Tue, Jun 19, 2018 at 05:40:26PM +0200, Ard Biesheuvel wrote:
> On 19 June 2018 at 17:37, Will Deacon <will.deacon@arm.com> wrote:
> > On Tue, Jun 19, 2018 at 05:29:03PM +0200, Ard Biesheuvel wrote:
> >> On 19 June 2018 at 17:28, Will Deacon <will.deacon@arm.com> wrote:
> >> > On Tue, Jun 19, 2018 at 05:23:41PM +0200, Ard Biesheuvel wrote:
> >> >> On 19 June 2018 at 17:20, Will Deacon <will.deacon@arm.com> wrote:
> >> >> > Hi Ard,
> >> >> >
> >> >> > Sorry, I forgot to reply to this.
> >> >> >
> >> >> > On Wed, May 30, 2018 at 11:53:20AM +0200, Ard Biesheuvel wrote:
> >> >> >> On 30 May 2018 at 11:14, Will Deacon <will.deacon@arm.com> wrote:
> >> >> >> > On Wed, May 30, 2018 at 12:48:06PM +0800, YaoJun wrote:
> >> >> >> >> To protect against KSMA(Kernel Space Mirroring Attack), make
> >> >> >> >> tramp_pg_dir read-only. The principle of KSMA is to insert a
> >> >> >> >> carefully constructed PGD entry into the translation table.
> >> >> >> >> The type of this entry is block, which maps the kernel text
> >> >> >> >> and its access permissions bits are 01. The user process can
> >> >> >> >> then modify kernel text directly through this mapping. In this
> >> >> >> >> way, an arbitrary write can be converted to multiple arbitrary
> >> >> >> >> writes.
> >> >> >> >>
> >> >> >> >> Signed-off-by: YaoJun <yaojun8558363@gmail.com>
> >> >> >> >> ---
> >> >> >> >>  arch/arm64/mm/mmu.c | 4 ++++
> >> >> >> >>  1 file changed, 4 insertions(+)
> >> >> >> >>
> >> >> >> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> >> >> >> >> index 2dbb2c9f1ec1..ac4b22c7e435 100644
> >> >> >> >> --- a/arch/arm64/mm/mmu.c
> >> >> >> >> +++ b/arch/arm64/mm/mmu.c
> >> >> >> >> @@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
> >> >> >> >>       __create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
> >> >> >> >>                            prot, pgd_pgtable_alloc, 0);
> >> >> >> >>
> >> >> >> >> +     update_mapping_prot(__pa_symbol(tramp_pg_dir),
> >> >> >> >> +                             (unsigned long)tramp_pg_dir,
> >> >> >> >> +                             PGD_SIZE, PAGE_KERNEL_RO);
> >> >> >> >
> >> >> >> > Hmm, I like the idea but is there a risk that the page table has been mapped
> >> >> >> > as part of a block entry, which we can't safely split at this point (i.e.
> >> >> >> > we'll run into one of the BUG_ONs in the mapping code)?
> >> >> >> >
> >> >> >>
> >> >> >> We'd need to create a separate segment for it initially so the mapping
> >> >> >> is already at the right granularity.
> >> >> >
> >> >> > Why do you think that's the case? I can't see anything that guarantees this
> >> >> > for the page table itself.
> >> >> >
> >> >>
> >> >> We'd need to pass NO_BLOCK_MAPPINGS to map_kernel_segment(),
> >> >> obviously, but that shouldn't hurt since that segment is relatively
> >> >> tiny anyway.
> >> >
> >> > Ah right, with NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS, I agree that we're good.
> >> > Ideally, we'd move {idmap,swapper,tramp}_pg_dir into .rodata...
> >> >
> >>
> >> idmap and tramp yes, but swapper needs to be modifiable at runtime, no?
> >
> > Right, but couldn't we swizzle the permissions in e.g. set_pmd? We could
> > even predicate that on a sanity check of the prot.
> >
> 
> Swizzle the permissions of the entire .rodata segment? That sounds
> doable, but there is a whole class of data that belongs in this
> category, and I think PaX/grsecurity had an API for that (but I don't
> think anyone is upstreaming that yet). So let's not reinvent that
> wheel for swapper_pg_dir only.

I wasn't thinking of the whole .rodata segment -- just the page containing
the entry being modified, but ok. I was hoping to avoid getting involved
with the PaX/grsec stuff as it seems rather toxic from what I've seen.

Will

Re: [PATCH] arm64: mm: mark tramp_pg_dir read-only

[Ard Biesheuvel]

On 19 June 2018 at 17:50, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Jun 19, 2018 at 05:40:26PM +0200, Ard Biesheuvel wrote:
>> On 19 June 2018 at 17:37, Will Deacon <will.deacon@arm.com> wrote:
>> > On Tue, Jun 19, 2018 at 05:29:03PM +0200, Ard Biesheuvel wrote:
>> >> On 19 June 2018 at 17:28, Will Deacon <will.deacon@arm.com> wrote:
>> >> > On Tue, Jun 19, 2018 at 05:23:41PM +0200, Ard Biesheuvel wrote:
>> >> >> On 19 June 2018 at 17:20, Will Deacon <will.deacon@arm.com> wrote:
>> >> >> > Hi Ard,
>> >> >> >
>> >> >> > Sorry, I forgot to reply to this.
>> >> >> >
>> >> >> > On Wed, May 30, 2018 at 11:53:20AM +0200, Ard Biesheuvel wrote:
>> >> >> >> On 30 May 2018 at 11:14, Will Deacon <will.deacon@arm.com> wrote:
>> >> >> >> > On Wed, May 30, 2018 at 12:48:06PM +0800, YaoJun wrote:
>> >> >> >> >> To protect against KSMA(Kernel Space Mirroring Attack), make
>> >> >> >> >> tramp_pg_dir read-only. The principle of KSMA is to insert a
>> >> >> >> >> carefully constructed PGD entry into the translation table.
>> >> >> >> >> The type of this entry is block, which maps the kernel text
>> >> >> >> >> and its access permissions bits are 01. The user process can
>> >> >> >> >> then modify kernel text directly through this mapping. In this
>> >> >> >> >> way, an arbitrary write can be converted to multiple arbitrary
>> >> >> >> >> writes.
>> >> >> >> >>
>> >> >> >> >> Signed-off-by: YaoJun <yaojun8558363@gmail.com>
>> >> >> >> >> ---
>> >> >> >> >>  arch/arm64/mm/mmu.c | 4 ++++
>> >> >> >> >>  1 file changed, 4 insertions(+)
>> >> >> >> >>
>> >> >> >> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> >> >> >> >> index 2dbb2c9f1ec1..ac4b22c7e435 100644
>> >> >> >> >> --- a/arch/arm64/mm/mmu.c
>> >> >> >> >> +++ b/arch/arm64/mm/mmu.c
>> >> >> >> >> @@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
>> >> >> >> >>       __create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
>> >> >> >> >>                            prot, pgd_pgtable_alloc, 0);
>> >> >> >> >>
>> >> >> >> >> +     update_mapping_prot(__pa_symbol(tramp_pg_dir),
>> >> >> >> >> +                             (unsigned long)tramp_pg_dir,
>> >> >> >> >> +                             PGD_SIZE, PAGE_KERNEL_RO);
>> >> >> >> >
>> >> >> >> > Hmm, I like the idea but is there a risk that the page table has been mapped
>> >> >> >> > as part of a block entry, which we can't safely split at this point (i.e.
>> >> >> >> > we'll run into one of the BUG_ONs in the mapping code)?
>> >> >> >> >
>> >> >> >>
>> >> >> >> We'd need to create a separate segment for it initially so the mapping
>> >> >> >> is already at the right granularity.
>> >> >> >
>> >> >> > Why do you think that's the case? I can't see anything that guarantees this
>> >> >> > for the page table itself.
>> >> >> >
>> >> >>
>> >> >> We'd need to pass NO_BLOCK_MAPPINGS to map_kernel_segment(),
>> >> >> obviously, but that shouldn't hurt since that segment is relatively
>> >> >> tiny anyway.
>> >> >
>> >> > Ah right, with NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS, I agree that we're good.
>> >> > Ideally, we'd move {idmap,swapper,tramp}_pg_dir into .rodata...
>> >> >
>> >>
>> >> idmap and tramp yes, but swapper needs to be modifiable at runtime, no?
>> >
>> > Right, but couldn't we swizzle the permissions in e.g. set_pmd? We could
>> > even predicate that on a sanity check of the prot.
>> >
>>
>> Swizzle the permissions of the entire .rodata segment? That sounds
>> doable, but there is a whole class of data that belongs in this
>> category, and I think PaX/grsecurity had an API for that (but I don't
>> think anyone is upstreaming that yet). So let's not reinvent that
>> wheel for swapper_pg_dir only.
>
> I wasn't thinking of the whole .rodata segment -- just the page containing
> the entry being modified, but ok.

That means we will need to map .rodata down to pages as well, or at
least avoid contiguous mappings.

> I was hoping to avoid getting involved
> with the PaX/grsec stuff as it seems rather toxic from what I've seen.
>

Indeed.

[PATCH] arm64: mm: mark tramp_pg_dir read-only

[Ard Biesheuvel]

On 19 June 2018 at 17:50, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Jun 19, 2018 at 05:40:26PM +0200, Ard Biesheuvel wrote:
>> On 19 June 2018 at 17:37, Will Deacon <will.deacon@arm.com> wrote:
>> > On Tue, Jun 19, 2018 at 05:29:03PM +0200, Ard Biesheuvel wrote:
>> >> On 19 June 2018 at 17:28, Will Deacon <will.deacon@arm.com> wrote:
>> >> > On Tue, Jun 19, 2018 at 05:23:41PM +0200, Ard Biesheuvel wrote:
>> >> >> On 19 June 2018 at 17:20, Will Deacon <will.deacon@arm.com> wrote:
>> >> >> > Hi Ard,
>> >> >> >
>> >> >> > Sorry, I forgot to reply to this.
>> >> >> >
>> >> >> > On Wed, May 30, 2018 at 11:53:20AM +0200, Ard Biesheuvel wrote:
>> >> >> >> On 30 May 2018 at 11:14, Will Deacon <will.deacon@arm.com> wrote:
>> >> >> >> > On Wed, May 30, 2018 at 12:48:06PM +0800, YaoJun wrote:
>> >> >> >> >> To protect against KSMA(Kernel Space Mirroring Attack), make
>> >> >> >> >> tramp_pg_dir read-only. The principle of KSMA is to insert a
>> >> >> >> >> carefully constructed PGD entry into the translation table.
>> >> >> >> >> The type of this entry is block, which maps the kernel text
>> >> >> >> >> and its access permissions bits are 01. The user process can
>> >> >> >> >> then modify kernel text directly through this mapping. In this
>> >> >> >> >> way, an arbitrary write can be converted to multiple arbitrary
>> >> >> >> >> writes.
>> >> >> >> >>
>> >> >> >> >> Signed-off-by: YaoJun <yaojun8558363@gmail.com>
>> >> >> >> >> ---
>> >> >> >> >>  arch/arm64/mm/mmu.c | 4 ++++
>> >> >> >> >>  1 file changed, 4 insertions(+)
>> >> >> >> >>
>> >> >> >> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> >> >> >> >> index 2dbb2c9f1ec1..ac4b22c7e435 100644
>> >> >> >> >> --- a/arch/arm64/mm/mmu.c
>> >> >> >> >> +++ b/arch/arm64/mm/mmu.c
>> >> >> >> >> @@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
>> >> >> >> >>       __create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
>> >> >> >> >>                            prot, pgd_pgtable_alloc, 0);
>> >> >> >> >>
>> >> >> >> >> +     update_mapping_prot(__pa_symbol(tramp_pg_dir),
>> >> >> >> >> +                             (unsigned long)tramp_pg_dir,
>> >> >> >> >> +                             PGD_SIZE, PAGE_KERNEL_RO);
>> >> >> >> >
>> >> >> >> > Hmm, I like the idea but is there a risk that the page table has been mapped
>> >> >> >> > as part of a block entry, which we can't safely split at this point (i.e.
>> >> >> >> > we'll run into one of the BUG_ONs in the mapping code)?
>> >> >> >> >
>> >> >> >>
>> >> >> >> We'd need to create a separate segment for it initially so the mapping
>> >> >> >> is already at the right granularity.
>> >> >> >
>> >> >> > Why do you think that's the case? I can't see anything that guarantees this
>> >> >> > for the page table itself.
>> >> >> >
>> >> >>
>> >> >> We'd need to pass NO_BLOCK_MAPPINGS to map_kernel_segment(),
>> >> >> obviously, but that shouldn't hurt since that segment is relatively
>> >> >> tiny anyway.
>> >> >
>> >> > Ah right, with NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS, I agree that we're good.
>> >> > Ideally, we'd move {idmap,swapper,tramp}_pg_dir into .rodata...
>> >> >
>> >>
>> >> idmap and tramp yes, but swapper needs to be modifiable at runtime, no?
>> >
>> > Right, but couldn't we swizzle the permissions in e.g. set_pmd? We could
>> > even predicate that on a sanity check of the prot.
>> >
>>
>> Swizzle the permissions of the entire .rodata segment? That sounds
>> doable, but there is a whole class of data that belongs in this
>> category, and I think PaX/grsecurity had an API for that (but I don't
>> think anyone is upstreaming that yet). So let's not reinvent that
>> wheel for swapper_pg_dir only.
>
> I wasn't thinking of the whole .rodata segment -- just the page containing
> the entry being modified, but ok.

That means we will need to map .rodata down to pages as well, or at
least avoid contiguous mappings.

> I was hoping to avoid getting involved
> with the PaX/grsec stuff as it seems rather toxic from what I've seen.
>

Indeed.

Re: [PATCH] arm64: mm: mark tramp_pg_dir read-only

[Will Deacon]

On Tue, Jun 19, 2018 at 05:51:46PM +0200, Ard Biesheuvel wrote:
> On 19 June 2018 at 17:50, Will Deacon <will.deacon@arm.com> wrote:
> > On Tue, Jun 19, 2018 at 05:40:26PM +0200, Ard Biesheuvel wrote:
> >> On 19 June 2018 at 17:37, Will Deacon <will.deacon@arm.com> wrote:
> >> > On Tue, Jun 19, 2018 at 05:29:03PM +0200, Ard Biesheuvel wrote:
> >> >> On 19 June 2018 at 17:28, Will Deacon <will.deacon@arm.com> wrote:
> >> >> > On Tue, Jun 19, 2018 at 05:23:41PM +0200, Ard Biesheuvel wrote:
> >> >> >> On 19 June 2018 at 17:20, Will Deacon <will.deacon@arm.com> wrote:
> >> >> >> > On Wed, May 30, 2018 at 11:53:20AM +0200, Ard Biesheuvel wrote:
> >> >> >> >> On 30 May 2018 at 11:14, Will Deacon <will.deacon@arm.com> wrote:
> >> >> >> >> > On Wed, May 30, 2018 at 12:48:06PM +0800, YaoJun wrote:
> >> >> >> >> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> >> >> >> >> >> index 2dbb2c9f1ec1..ac4b22c7e435 100644
> >> >> >> >> >> --- a/arch/arm64/mm/mmu.c
> >> >> >> >> >> +++ b/arch/arm64/mm/mmu.c
> >> >> >> >> >> @@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
> >> >> >> >> >>       __create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
> >> >> >> >> >>                            prot, pgd_pgtable_alloc, 0);
> >> >> >> >> >>
> >> >> >> >> >> +     update_mapping_prot(__pa_symbol(tramp_pg_dir),
> >> >> >> >> >> +                             (unsigned long)tramp_pg_dir,
> >> >> >> >> >> +                             PGD_SIZE, PAGE_KERNEL_RO);
> >> >> >> >> >
> >> >> >> >> > Hmm, I like the idea but is there a risk that the page table has been mapped
> >> >> >> >> > as part of a block entry, which we can't safely split at this point (i.e.
> >> >> >> >> > we'll run into one of the BUG_ONs in the mapping code)?
> >> >> >> >> >
> >> >> >> >>
> >> >> >> >> We'd need to create a separate segment for it initially so the mapping
> >> >> >> >> is already at the right granularity.
> >> >> >> >
> >> >> >> > Why do you think that's the case? I can't see anything that guarantees this
> >> >> >> > for the page table itself.
> >> >> >> >
> >> >> >>
> >> >> >> We'd need to pass NO_BLOCK_MAPPINGS to map_kernel_segment(),
> >> >> >> obviously, but that shouldn't hurt since that segment is relatively
> >> >> >> tiny anyway.
> >> >> >
> >> >> > Ah right, with NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS, I agree that we're good.
> >> >> > Ideally, we'd move {idmap,swapper,tramp}_pg_dir into .rodata...
> >> >> >
> >> >>
> >> >> idmap and tramp yes, but swapper needs to be modifiable at runtime, no?
> >> >
> >> > Right, but couldn't we swizzle the permissions in e.g. set_pmd? We could
> >> > even predicate that on a sanity check of the prot.
> >> >
> >>
> >> Swizzle the permissions of the entire .rodata segment? That sounds
> >> doable, but there is a whole class of data that belongs in this
> >> category, and I think PaX/grsecurity had an API for that (but I don't
> >> think anyone is upstreaming that yet). So let's not reinvent that
> >> wheel for swapper_pg_dir only.
> >
> > I wasn't thinking of the whole .rodata segment -- just the page containing
> > the entry being modified, but ok.
> 
> That means we will need to map .rodata down to pages as well, or at
> least avoid contiguous mappings.

Doesn't it already avoid those?

Will

[PATCH] arm64: mm: mark tramp_pg_dir read-only

[Will Deacon]

On Tue, Jun 19, 2018 at 05:51:46PM +0200, Ard Biesheuvel wrote:
> On 19 June 2018 at 17:50, Will Deacon <will.deacon@arm.com> wrote:
> > On Tue, Jun 19, 2018 at 05:40:26PM +0200, Ard Biesheuvel wrote:
> >> On 19 June 2018 at 17:37, Will Deacon <will.deacon@arm.com> wrote:
> >> > On Tue, Jun 19, 2018 at 05:29:03PM +0200, Ard Biesheuvel wrote:
> >> >> On 19 June 2018 at 17:28, Will Deacon <will.deacon@arm.com> wrote:
> >> >> > On Tue, Jun 19, 2018 at 05:23:41PM +0200, Ard Biesheuvel wrote:
> >> >> >> On 19 June 2018 at 17:20, Will Deacon <will.deacon@arm.com> wrote:
> >> >> >> > On Wed, May 30, 2018 at 11:53:20AM +0200, Ard Biesheuvel wrote:
> >> >> >> >> On 30 May 2018 at 11:14, Will Deacon <will.deacon@arm.com> wrote:
> >> >> >> >> > On Wed, May 30, 2018 at 12:48:06PM +0800, YaoJun wrote:
> >> >> >> >> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> >> >> >> >> >> index 2dbb2c9f1ec1..ac4b22c7e435 100644
> >> >> >> >> >> --- a/arch/arm64/mm/mmu.c
> >> >> >> >> >> +++ b/arch/arm64/mm/mmu.c
> >> >> >> >> >> @@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
> >> >> >> >> >>       __create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
> >> >> >> >> >>                            prot, pgd_pgtable_alloc, 0);
> >> >> >> >> >>
> >> >> >> >> >> +     update_mapping_prot(__pa_symbol(tramp_pg_dir),
> >> >> >> >> >> +                             (unsigned long)tramp_pg_dir,
> >> >> >> >> >> +                             PGD_SIZE, PAGE_KERNEL_RO);
> >> >> >> >> >
> >> >> >> >> > Hmm, I like the idea but is there a risk that the page table has been mapped
> >> >> >> >> > as part of a block entry, which we can't safely split at this point (i.e.
> >> >> >> >> > we'll run into one of the BUG_ONs in the mapping code)?
> >> >> >> >> >
> >> >> >> >>
> >> >> >> >> We'd need to create a separate segment for it initially so the mapping
> >> >> >> >> is already at the right granularity.
> >> >> >> >
> >> >> >> > Why do you think that's the case? I can't see anything that guarantees this
> >> >> >> > for the page table itself.
> >> >> >> >
> >> >> >>
> >> >> >> We'd need to pass NO_BLOCK_MAPPINGS to map_kernel_segment(),
> >> >> >> obviously, but that shouldn't hurt since that segment is relatively
> >> >> >> tiny anyway.
> >> >> >
> >> >> > Ah right, with NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS, I agree that we're good.
> >> >> > Ideally, we'd move {idmap,swapper,tramp}_pg_dir into .rodata...
> >> >> >
> >> >>
> >> >> idmap and tramp yes, but swapper needs to be modifiable at runtime, no?
> >> >
> >> > Right, but couldn't we swizzle the permissions in e.g. set_pmd? We could
> >> > even predicate that on a sanity check of the prot.
> >> >
> >>
> >> Swizzle the permissions of the entire .rodata segment? That sounds
> >> doable, but there is a whole class of data that belongs in this
> >> category, and I think PaX/grsecurity had an API for that (but I don't
> >> think anyone is upstreaming that yet). So let's not reinvent that
> >> wheel for swapper_pg_dir only.
> >
> > I wasn't thinking of the whole .rodata segment -- just the page containing
> > the entry being modified, but ok.
> 
> That means we will need to map .rodata down to pages as well, or at
> least avoid contiguous mappings.

Doesn't it already avoid those?

Will

Re: [PATCH] arm64: mm: mark tramp_pg_dir read-only

[Ard Biesheuvel]

On 19 June 2018 at 17:55, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Jun 19, 2018 at 05:51:46PM +0200, Ard Biesheuvel wrote:
>> On 19 June 2018 at 17:50, Will Deacon <will.deacon@arm.com> wrote:
>> > On Tue, Jun 19, 2018 at 05:40:26PM +0200, Ard Biesheuvel wrote:
>> >> On 19 June 2018 at 17:37, Will Deacon <will.deacon@arm.com> wrote:
>> >> > On Tue, Jun 19, 2018 at 05:29:03PM +0200, Ard Biesheuvel wrote:
>> >> >> On 19 June 2018 at 17:28, Will Deacon <will.deacon@arm.com> wrote:
>> >> >> > On Tue, Jun 19, 2018 at 05:23:41PM +0200, Ard Biesheuvel wrote:
>> >> >> >> On 19 June 2018 at 17:20, Will Deacon <will.deacon@arm.com> wrote:
>> >> >> >> > On Wed, May 30, 2018 at 11:53:20AM +0200, Ard Biesheuvel wrote:
>> >> >> >> >> On 30 May 2018 at 11:14, Will Deacon <will.deacon@arm.com> wrote:
>> >> >> >> >> > On Wed, May 30, 2018 at 12:48:06PM +0800, YaoJun wrote:
>> >> >> >> >> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> >> >> >> >> >> index 2dbb2c9f1ec1..ac4b22c7e435 100644
>> >> >> >> >> >> --- a/arch/arm64/mm/mmu.c
>> >> >> >> >> >> +++ b/arch/arm64/mm/mmu.c
>> >> >> >> >> >> @@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
>> >> >> >> >> >>       __create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
>> >> >> >> >> >>                            prot, pgd_pgtable_alloc, 0);
>> >> >> >> >> >>
>> >> >> >> >> >> +     update_mapping_prot(__pa_symbol(tramp_pg_dir),
>> >> >> >> >> >> +                             (unsigned long)tramp_pg_dir,
>> >> >> >> >> >> +                             PGD_SIZE, PAGE_KERNEL_RO);
>> >> >> >> >> >
>> >> >> >> >> > Hmm, I like the idea but is there a risk that the page table has been mapped
>> >> >> >> >> > as part of a block entry, which we can't safely split at this point (i.e.
>> >> >> >> >> > we'll run into one of the BUG_ONs in the mapping code)?
>> >> >> >> >> >
>> >> >> >> >>
>> >> >> >> >> We'd need to create a separate segment for it initially so the mapping
>> >> >> >> >> is already at the right granularity.
>> >> >> >> >
>> >> >> >> > Why do you think that's the case? I can't see anything that guarantees this
>> >> >> >> > for the page table itself.
>> >> >> >> >
>> >> >> >>
>> >> >> >> We'd need to pass NO_BLOCK_MAPPINGS to map_kernel_segment(),
>> >> >> >> obviously, but that shouldn't hurt since that segment is relatively
>> >> >> >> tiny anyway.
>> >> >> >
>> >> >> > Ah right, with NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS, I agree that we're good.
>> >> >> > Ideally, we'd move {idmap,swapper,tramp}_pg_dir into .rodata...
>> >> >> >
>> >> >>
>> >> >> idmap and tramp yes, but swapper needs to be modifiable at runtime, no?
>> >> >
>> >> > Right, but couldn't we swizzle the permissions in e.g. set_pmd? We could
>> >> > even predicate that on a sanity check of the prot.
>> >> >
>> >>
>> >> Swizzle the permissions of the entire .rodata segment? That sounds
>> >> doable, but there is a whole class of data that belongs in this
>> >> category, and I think PaX/grsecurity had an API for that (but I don't
>> >> think anyone is upstreaming that yet). So let's not reinvent that
>> >> wheel for swapper_pg_dir only.
>> >
>> > I wasn't thinking of the whole .rodata segment -- just the page containing
>> > the entry being modified, but ok.
>>
>> That means we will need to map .rodata down to pages as well, or at
>> least avoid contiguous mappings.
>
> Doesn't it already avoid those?
>

You are right, it does.

[PATCH] arm64: mm: mark tramp_pg_dir read-only

[Ard Biesheuvel]

On 19 June 2018 at 17:55, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Jun 19, 2018 at 05:51:46PM +0200, Ard Biesheuvel wrote:
>> On 19 June 2018 at 17:50, Will Deacon <will.deacon@arm.com> wrote:
>> > On Tue, Jun 19, 2018 at 05:40:26PM +0200, Ard Biesheuvel wrote:
>> >> On 19 June 2018 at 17:37, Will Deacon <will.deacon@arm.com> wrote:
>> >> > On Tue, Jun 19, 2018 at 05:29:03PM +0200, Ard Biesheuvel wrote:
>> >> >> On 19 June 2018 at 17:28, Will Deacon <will.deacon@arm.com> wrote:
>> >> >> > On Tue, Jun 19, 2018 at 05:23:41PM +0200, Ard Biesheuvel wrote:
>> >> >> >> On 19 June 2018 at 17:20, Will Deacon <will.deacon@arm.com> wrote:
>> >> >> >> > On Wed, May 30, 2018 at 11:53:20AM +0200, Ard Biesheuvel wrote:
>> >> >> >> >> On 30 May 2018 at 11:14, Will Deacon <will.deacon@arm.com> wrote:
>> >> >> >> >> > On Wed, May 30, 2018 at 12:48:06PM +0800, YaoJun wrote:
>> >> >> >> >> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> >> >> >> >> >> index 2dbb2c9f1ec1..ac4b22c7e435 100644
>> >> >> >> >> >> --- a/arch/arm64/mm/mmu.c
>> >> >> >> >> >> +++ b/arch/arm64/mm/mmu.c
>> >> >> >> >> >> @@ -551,6 +551,10 @@ static int __init map_entry_trampoline(void)
>> >> >> >> >> >>       __create_pgd_mapping(tramp_pg_dir, pa_start, TRAMP_VALIAS, PAGE_SIZE,
>> >> >> >> >> >>                            prot, pgd_pgtable_alloc, 0);
>> >> >> >> >> >>
>> >> >> >> >> >> +     update_mapping_prot(__pa_symbol(tramp_pg_dir),
>> >> >> >> >> >> +                             (unsigned long)tramp_pg_dir,
>> >> >> >> >> >> +                             PGD_SIZE, PAGE_KERNEL_RO);
>> >> >> >> >> >
>> >> >> >> >> > Hmm, I like the idea but is there a risk that the page table has been mapped
>> >> >> >> >> > as part of a block entry, which we can't safely split at this point (i.e.
>> >> >> >> >> > we'll run into one of the BUG_ONs in the mapping code)?
>> >> >> >> >> >
>> >> >> >> >>
>> >> >> >> >> We'd need to create a separate segment for it initially so the mapping
>> >> >> >> >> is already at the right granularity.
>> >> >> >> >
>> >> >> >> > Why do you think that's the case? I can't see anything that guarantees this
>> >> >> >> > for the page table itself.
>> >> >> >> >
>> >> >> >>
>> >> >> >> We'd need to pass NO_BLOCK_MAPPINGS to map_kernel_segment(),
>> >> >> >> obviously, but that shouldn't hurt since that segment is relatively
>> >> >> >> tiny anyway.
>> >> >> >
>> >> >> > Ah right, with NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS, I agree that we're good.
>> >> >> > Ideally, we'd move {idmap,swapper,tramp}_pg_dir into .rodata...
>> >> >> >
>> >> >>
>> >> >> idmap and tramp yes, but swapper needs to be modifiable at runtime, no?
>> >> >
>> >> > Right, but couldn't we swizzle the permissions in e.g. set_pmd? We could
>> >> > even predicate that on a sanity check of the prot.
>> >> >
>> >>
>> >> Swizzle the permissions of the entire .rodata segment? That sounds
>> >> doable, but there is a whole class of data that belongs in this
>> >> category, and I think PaX/grsecurity had an API for that (but I don't
>> >> think anyone is upstreaming that yet). So let's not reinvent that
>> >> wheel for swapper_pg_dir only.
>> >
>> > I wasn't thinking of the whole .rodata segment -- just the page containing
>> > the entry being modified, but ok.
>>
>> That means we will need to map .rodata down to pages as well, or at
>> least avoid contiguous mappings.
>
> Doesn't it already avoid those?
>

You are right, it does.