[PATCH 1/2] powerpc/strict_rwx: quirks for STRICT_RWX patches

[PATCH 1/2] powerpc/strict_rwx: quirks for STRICT_RWX patches

[Balbir Singh]

This patch disables STRICT_RWX for power9 DD1 machines
where due to some limitations with the way we do tlb
updates, we clear the TLB entry of the text that's doing
the update to kernel text and that does lead to a
crash.

Fixes: 7614ff3 ("powerpc/mm/radix: Implement STRICT_RWX/mark_rodata_ro() for Radix")

Cc: stable@vger.kernel.org

Reported-by: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Balbir Singh <bsingharora@gmail.com>
---
 arch/powerpc/mm/pgtable-radix.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
index 39c252b..c2a2b46 100644
--- a/arch/powerpc/mm/pgtable-radix.c
+++ b/arch/powerpc/mm/pgtable-radix.c
@@ -169,6 +169,18 @@ void radix__mark_rodata_ro(void)
 {
 	unsigned long start, end;
 
+	/*
+	 * mark_rodata_ro() will mark itself as !writable at some point
+	 * due to workaround in radix__pte_update(), we'll end up with
+	 * an invalid pte and the system will crash quite severly.
+	 * The alternatives are quite cumbersome and leaving out
+	 * the page containing the flush code is not very nice.
+	 */
+	if (cpu_has_feature(CPU_FTR_POWER9_DD1)) {
+		pr_warn("Warning: Unable to mark rodata read only on P9 DD1\n");
+		return;
+	}
+
 	start = (unsigned long)_stext;
 	end = (unsigned long)__init_begin;
 
-- 
2.9.5

[PATCH 2/2] powerpc/strict_rwx: fixup region splitting

[Balbir Singh]

We were aggressive with splitting regions and missed the case
when _stext and __init_begin completely overlap addr and addr+mapping.

This patch fixes that case and allows us to keep the largest possible
mapping through the overlap. The patch also simplies the check
into a function

Fixes: 7614ff3 ("powerpc/mm/radix: Implement STRICT_RWX/mark_rodata_ro() for Radix")

Signed-off-by: Balbir Singh <bsingharora@gmail.com>
---
 arch/powerpc/mm/pgtable-radix.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
index c2a2b46..ea0da3b 100644
--- a/arch/powerpc/mm/pgtable-radix.c
+++ b/arch/powerpc/mm/pgtable-radix.c
@@ -210,17 +210,36 @@ static inline void __meminit print_mapping(unsigned long start,
 	pr_info("Mapped 0x%016lx-0x%016lx with %s pages\n", start, end, buf);
 }
 
+static inline int __meminit
+should_split_mapping_size(unsigned long addr, unsigned long mapping_size)
+{
+#ifdef CONFIG_STRICT_KERNEL_RWX
+	/*
+	 * If addr, addr+mapping overlap the text region and
+	 * _stext and __init_begin end up lying between
+	 * addr, addr+mapping split the mapping size down
+	 * further
+	 */
+	if ((addr < __pa_symbol(__init_begin)) &&
+		(addr + mapping_size) > __pa_symbol(_stext)) {
+
+		if (((addr + mapping_size) <= __pa_symbol(__init_begin)) &&
+			(addr >= __pa_symbol(_stext)))
+			return 0;
+
+		return 1;
+	}
+#endif
+	return 0;
+}
+
+
 static int __meminit create_physical_mapping(unsigned long start,
 					     unsigned long end)
 {
 	unsigned long vaddr, addr, mapping_size = 0;
 	pgprot_t prot;
 	unsigned long max_mapping_size;
-#ifdef CONFIG_STRICT_KERNEL_RWX
-	int split_text_mapping = 1;
-#else
-	int split_text_mapping = 0;
-#endif
 
 	start = _ALIGN_UP(start, PAGE_SIZE);
 	for (addr = start; addr < end; addr += mapping_size) {
@@ -242,16 +261,14 @@ static int __meminit create_physical_mapping(unsigned long start,
 		else
 			mapping_size = PAGE_SIZE;
 
-		if (split_text_mapping && (mapping_size == PUD_SIZE) &&
-			(addr <= __pa_symbol(__init_begin)) &&
-			(addr + mapping_size) >= __pa_symbol(_stext)) {
+		if ((mapping_size == PUD_SIZE) &&
+			should_split_mapping_size(addr, mapping_size)) {
 			max_mapping_size = PMD_SIZE;
 			goto retry;
 		}
 
-		if (split_text_mapping && (mapping_size == PMD_SIZE) &&
-		    (addr <= __pa_symbol(__init_begin)) &&
-		    (addr + mapping_size) >= __pa_symbol(_stext))
+		if ((mapping_size == PMD_SIZE) &&
+			should_split_mapping_size(addr, mapping_size))
 			mapping_size = PAGE_SIZE;
 
 		if (mapping_size != previous_size) {
-- 
2.9.5

Re: [PATCH 1/2] powerpc/strict_rwx: quirks for STRICT_RWX patches

[Michael Ellerman]

Hi Balbir,

Mainly I think we need to add a check in mark_initmem_nx() too don't we?
Or should we put it somewhere common to both?

But seeing as I'm replying here are some more comments.

> Subject: [PATCH 1/2] powerpc/strict_rwx: quirks for STRICT_RWX patches

This misses three key things, 1) it's a bug fix 2) it's for Power9 DD1,
and 3) it only applies to radix.

So how about:

powerpc/64s: Fix crashes on Power9 DD1 with radix MMU and STRICT_RWX


Balbir Singh <bsingharora@gmail.com> writes:
> This patch disables STRICT_RWX for power9 DD1 machines
> where due to some limitations with the way we do tlb
> updates, we clear the TLB entry of the text that's doing
> the update to kernel text and that does lead to a
> crash.

This mostly describes the patch, but I would prefer more detail, eg:

  When using the radix MMU on Power9 DD1, to work around a hardware
  problem, radix__pte_update() is required to do a two stage update of
  the PTE. First we write a zero value into the PTE, then we flush the
  TLB, and then we write the new PTE value.
  
  In the normal case that works OK, but it does not work if we're
  updating the PTE that maps the code we're executing, because the
  mapping is removed by the TLB flush and we can no longer execute from
  it. Unfortunately the STRICT_RWX code needs to do exactly that.
  
  The exact symptoms when we hit this case vary, sometimes we print an
  oops and then get stuck after that, but I've also seen a machine just
  get stuck continually page faulting with no oops printed. The variance
  is presumably due to the exact layout of the text and the page size
  used for the mappings. In all cases we are unable to boot to a shell.
  
  There are possible solutions such as creating a second mapping of the
  TLB flush code, executing from that, and then jumping back to the
  original. However we don't want to add that level of complexity for a
  DD1 work around.
  
  So just detect that we're running on Power9 DD1 and refrain from
  changing the permissions, effectively disabling STRICT_RWX on Power9
  DD1.


> diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
> index 39c252b..c2a2b46 100644
> --- a/arch/powerpc/mm/pgtable-radix.c
> +++ b/arch/powerpc/mm/pgtable-radix.c
> @@ -169,6 +169,18 @@ void radix__mark_rodata_ro(void)
>  {
>  	unsigned long start, end;
>  
> +	/*
> +	 * mark_rodata_ro() will mark itself as !writable at some point
                                                                       ^
                                                                       .
> +	 * due to workaround in radix__pte_update(), we'll end up with
           ^     ^
           D     the DD1
> +	 * an invalid pte and the system will crash quite severly.

> +	 * The alternatives are quite cumbersome and leaving out
> +	 * the page containing the flush code is not very nice.

Just drop the last sentence, it doesn't have enough detail to be useful
and we chose not to implement those alternatives, so better not to
confuse the reader by talking about them.

> +	 */
> +	if (cpu_has_feature(CPU_FTR_POWER9_DD1)) {
> +		pr_warn("Warning: Unable to mark rodata read only on P9 DD1\n");
> +		return;
> +	}

Don't we also need to do the check in radix__mark_initmem_nx() ?

cheers

Re: [PATCH 2/2] powerpc/strict_rwx: fixup region splitting

[Michael Ellerman]

Balbir Singh <bsingharora@gmail.com> writes:

> We were aggressive with splitting regions and missed the case
> when _stext and __init_begin completely overlap addr and addr+mapping.
>
> This patch fixes that case and allows us to keep the largest possible
> mapping through the overlap. The patch also simplies the check
> into a function

Please do the fix in one patch, which we can backport if necessary, and
then move it into a function in a separate patch.

cheers

Re: [PATCH 1/2] powerpc/strict_rwx: quirks for STRICT_RWX patches

[Balbir Singh]

On Wed, 04 Oct 2017 20:04:52 +1100
Michael Ellerman <mpe@ellerman.id.au> wrote:

> Hi Balbir,
> 
> Mainly I think we need to add a check in mark_initmem_nx() too don't we?
> Or should we put it somewhere common to both?
> 
> But seeing as I'm replying here are some more comments.
> 
> > Subject: [PATCH 1/2] powerpc/strict_rwx: quirks for STRICT_RWX patches  
> 
> This misses three key things, 1) it's a bug fix 2) it's for Power9 DD1,
> and 3) it only applies to radix.
> 
> So how about:
> 
> powerpc/64s: Fix crashes on Power9 DD1 with radix MMU and STRICT_RWX
> 
> 
> Balbir Singh <bsingharora@gmail.com> writes:
> > This patch disables STRICT_RWX for power9 DD1 machines
> > where due to some limitations with the way we do tlb
> > updates, we clear the TLB entry of the text that's doing
> > the update to kernel text and that does lead to a
> > crash.  
> 
> This mostly describes the patch, but I would prefer more detail, eg:
> 
>   When using the radix MMU on Power9 DD1, to work around a hardware
>   problem, radix__pte_update() is required to do a two stage update of
>   the PTE. First we write a zero value into the PTE, then we flush the
>   TLB, and then we write the new PTE value.
>   
>   In the normal case that works OK, but it does not work if we're
>   updating the PTE that maps the code we're executing, because the
>   mapping is removed by the TLB flush and we can no longer execute from
>   it. Unfortunately the STRICT_RWX code needs to do exactly that.
>   
>   The exact symptoms when we hit this case vary, sometimes we print an
>   oops and then get stuck after that, but I've also seen a machine just
>   get stuck continually page faulting with no oops printed. The variance
>   is presumably due to the exact layout of the text and the page size
>   used for the mappings. In all cases we are unable to boot to a shell.
>   
>   There are possible solutions such as creating a second mapping of the
>   TLB flush code, executing from that, and then jumping back to the
>   original. However we don't want to add that level of complexity for a
>   DD1 work around.
>   
>   So just detect that we're running on Power9 DD1 and refrain from
>   changing the permissions, effectively disabling STRICT_RWX on Power9
>   DD1.
>

I can copy this verbatim

> 
> > diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
> > index 39c252b..c2a2b46 100644
> > --- a/arch/powerpc/mm/pgtable-radix.c
> > +++ b/arch/powerpc/mm/pgtable-radix.c
> > @@ -169,6 +169,18 @@ void radix__mark_rodata_ro(void)
> >  {
> >  	unsigned long start, end;
> >  
> > +	/*
> > +	 * mark_rodata_ro() will mark itself as !writable at some point  
>                                                                        ^
>                                                                        .
> > +	 * due to workaround in radix__pte_update(), we'll end up with  
>            ^     ^
>            D     the DD1
> > +	 * an invalid pte and the system will crash quite severly.  
> 
> > +	 * The alternatives are quite cumbersome and leaving out
> > +	 * the page containing the flush code is not very nice.  
> 
> Just drop the last sentence, it doesn't have enough detail to be useful
> and we chose not to implement those alternatives, so better not to
> confuse the reader by talking about them.
> 

Can and will do

> > +	 */
> > +	if (cpu_has_feature(CPU_FTR_POWER9_DD1)) {
> > +		pr_warn("Warning: Unable to mark rodata read only on P9 DD1\n");
> > +		return;
> > +	}  
> 
> Don't we also need to do the check in radix__mark_initmem_nx() ?

No.. not for NX, since we mark NX pages (.data and .init.text..init.end)
from .text. We don't change permissions for anything in .text in that
routine.

> 
> cheers

Re: [PATCH 2/2] powerpc/strict_rwx: fixup region splitting

[Balbir Singh]

On Wed, 04 Oct 2017 22:14:17 +1100
Michael Ellerman <mpe@ellerman.id.au> wrote:

> Balbir Singh <bsingharora@gmail.com> writes:
> 
> > We were aggressive with splitting regions and missed the case
> > when _stext and __init_begin completely overlap addr and addr+mapping.
> >
> > This patch fixes that case and allows us to keep the largest possible
> > mapping through the overlap. The patch also simplies the check
> > into a function  
> 
> Please do the fix in one patch, which we can backport if necessary, and
> then move it into a function in a separate patch.
> 

I've tried for a while now, although this looks like its touching more
code, this is more elegant. Anything else involves changing the two
if conditions almost identically.

Balbir Singh.