Re: [PATCH v1 1/5] powerpc/mm: sanity_check_fault() should work for all, not only BOOK3S

2020-09-08 Thread Christophe Leroy 




Le 08/09/2020 à 10:43, Nicholas Piggin a écrit :

Excerpts from Christophe Leroy's message of August 7, 2020 3:15 am:

The verification and message introduced by commit 374f3f5979f9
("powerpc/mm/hash: Handle user access of kernel address gracefully")
applies to all platforms, it should not be limited to BOOK3S.

Make the BOOK3S version of sanity_check_fault() the one for all,
and bail out earlier if not BOOK3S.

Fixes: 374f3f5979f9 ("powerpc/mm/hash: Handle user access of kernel address 
gracefully")
Signed-off-by: Christophe Leroy 
---
  arch/powerpc/mm/fault.c | 8 +++-
  1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 925a7231abb3..2efa34d7e644 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -303,7 +303,6 @@ static inline void cmo_account_page_fault(void)
  static inline void cmo_account_page_fault(void) { }
  #endif /* CONFIG_PPC_SMLPAR */
  
-#ifdef CONFIG_PPC_BOOK3S

  static void sanity_check_fault(bool is_write, bool is_user,
   unsigned long error_code, unsigned long address)
  {
@@ -320,6 +319,9 @@ static void sanity_check_fault(bool is_write, bool is_user,
return;
}
  
+	if (!IS_ENABLED(CONFIG_PPC_BOOK3S))

+   return;


Seems okay. Why is address == -1 special though? I guess it's because
it may not be an exploit kernel reference but a buggy pointer underflow?
In that case -1 doesn't seem like it would catch very much. Would it be
better to test for high bit set for example ((long)address < 0) ?


See 
https://github.com/linuxppc/linux/commit/0f9aee0cb9da7db7d96f63cfa2dc5e4f1bffeb87#diff-f9658f412252f3bb3093e0a95b37f3ac


-1 is what mmap() returns on error, if the app uses that as a pointer 
that's a programming error not an exploit.


Euh .. If you test (long)address < 0, then the entire kernel falls into 
that range as usually it goes from 0xc000 to 0x


But we could skip the top page entirely, anyway it is never mapped.



Anyway for your patch

Reviewed-by: Nicholas Piggin 


Thanks
Christophe

Re: [PATCH v1 1/5] powerpc/mm: sanity_check_fault() should work for all, not only BOOK3S

2020-09-08 Thread Nicholas Piggin 
Excerpts from Christophe Leroy's message of August 7, 2020 3:15 am:
> The verification and message introduced by commit 374f3f5979f9
> ("powerpc/mm/hash: Handle user access of kernel address gracefully")
> applies to all platforms, it should not be limited to BOOK3S.
> 
> Make the BOOK3S version of sanity_check_fault() the one for all,
> and bail out earlier if not BOOK3S.
> 
> Fixes: 374f3f5979f9 ("powerpc/mm/hash: Handle user access of kernel address 
> gracefully")
> Signed-off-by: Christophe Leroy 
> ---
>  arch/powerpc/mm/fault.c | 8 +++-
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 925a7231abb3..2efa34d7e644 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -303,7 +303,6 @@ static inline void cmo_account_page_fault(void)
>  static inline void cmo_account_page_fault(void) { }
>  #endif /* CONFIG_PPC_SMLPAR */
>  
> -#ifdef CONFIG_PPC_BOOK3S
>  static void sanity_check_fault(bool is_write, bool is_user,
>  unsigned long error_code, unsigned long address)
>  {
> @@ -320,6 +319,9 @@ static void sanity_check_fault(bool is_write, bool 
> is_user,
>   return;
>   }
>  
> + if (!IS_ENABLED(CONFIG_PPC_BOOK3S))
> + return;

Seems okay. Why is address == -1 special though? I guess it's because 
it may not be an exploit kernel reference but a buggy pointer underflow? 
In that case -1 doesn't seem like it would catch very much. Would it be 
better to test for high bit set for example ((long)address < 0) ?

Anyway for your patch

Reviewed-by: Nicholas Piggin 

> +
>   /*
>* For hash translation mode, we should never get a
>* PROTFAULT. Any update to pte to reduce access will result in us
> @@ -354,10 +356,6 @@ static void sanity_check_fault(bool is_write, bool 
> is_user,
>  
>   WARN_ON_ONCE(error_code & DSISR_PROTFAULT);
>  }
> -#else
> -static void sanity_check_fault(bool is_write, bool is_user,
> -unsigned long error_code, unsigned long address) 
> { }
> -#endif /* CONFIG_PPC_BOOK3S */
>  
>  /*
>   * Define the correct "is_write" bit in error_code based
> -- 
> 2.25.0
> 
>

[PATCH v1 1/5] powerpc/mm: sanity_check_fault() should work for all, not only BOOK3S

2020-08-06 Thread Christophe Leroy 
The verification and message introduced by commit 374f3f5979f9
("powerpc/mm/hash: Handle user access of kernel address gracefully")
applies to all platforms, it should not be limited to BOOK3S.

Make the BOOK3S version of sanity_check_fault() the one for all,
and bail out earlier if not BOOK3S.

Fixes: 374f3f5979f9 ("powerpc/mm/hash: Handle user access of kernel address 
gracefully")
Signed-off-by: Christophe Leroy 
---
 arch/powerpc/mm/fault.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 925a7231abb3..2efa34d7e644 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -303,7 +303,6 @@ static inline void cmo_account_page_fault(void)
 static inline void cmo_account_page_fault(void) { }
 #endif /* CONFIG_PPC_SMLPAR */
 
-#ifdef CONFIG_PPC_BOOK3S
 static void sanity_check_fault(bool is_write, bool is_user,
   unsigned long error_code, unsigned long address)
 {
@@ -320,6 +319,9 @@ static void sanity_check_fault(bool is_write, bool is_user,
return;
}
 
+   if (!IS_ENABLED(CONFIG_PPC_BOOK3S))
+   return;
+
/*
 * For hash translation mode, we should never get a
 * PROTFAULT. Any update to pte to reduce access will result in us
@@ -354,10 +356,6 @@ static void sanity_check_fault(bool is_write, bool is_user,
 
WARN_ON_ONCE(error_code & DSISR_PROTFAULT);
 }
-#else
-static void sanity_check_fault(bool is_write, bool is_user,
-  unsigned long error_code, unsigned long address) 
{ }
-#endif /* CONFIG_PPC_BOOK3S */
 
 /*
  * Define the correct "is_write" bit in error_code based
-- 
2.25.0