* [PATCH v1 1/5] powerpc/mm: sanity_check_fault() should work for all, not only BOOK3S
@ 2020-08-06 17:15 ` Christophe Leroy
0 siblings, 0 replies; 29+ messages in thread
From: Christophe Leroy @ 2020-08-06 17:15 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, npiggin
Cc: linux-kernel, linuxppc-dev
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 <christophe.leroy@csgroup.eu>
---
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
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v1 1/5] powerpc/mm: sanity_check_fault() should work for all, not only BOOK3S
@ 2020-08-06 17:15 ` Christophe Leroy
0 siblings, 0 replies; 29+ messages in thread
From: Christophe Leroy @ 2020-08-06 17:15 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, npiggin
Cc: linuxppc-dev, linux-kernel
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 <christophe.leroy@csgroup.eu>
---
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
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v1 2/5] powerpc/fault: Unnest definition of page_fault_is_write() and page_fault_is_bad()
2020-08-06 17:15 ` Christophe Leroy
@ 2020-08-06 17:15 ` Christophe Leroy
-1 siblings, 0 replies; 29+ messages in thread
From: Christophe Leroy @ 2020-08-06 17:15 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, npiggin
Cc: linux-kernel, linuxppc-dev
To make it more readable, separate page_fault_is_write() and page_fault_is_bad()
to avoir several levels of #ifdefs
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/mm/fault.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 2efa34d7e644..9ef9ee244f72 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -363,17 +363,19 @@ static void sanity_check_fault(bool is_write, bool is_user,
*/
#if (defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
#define page_fault_is_write(__err) ((__err) & ESR_DST)
-#define page_fault_is_bad(__err) (0)
#else
#define page_fault_is_write(__err) ((__err) & DSISR_ISSTORE)
-#if defined(CONFIG_PPC_8xx)
+#endif
+
+#if defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
+#define page_fault_is_bad(__err) (0)
+#elif defined(CONFIG_PPC_8xx)
#define page_fault_is_bad(__err) ((__err) & DSISR_NOEXEC_OR_G)
#elif defined(CONFIG_PPC64)
#define page_fault_is_bad(__err) ((__err) & DSISR_BAD_FAULT_64S)
#else
#define page_fault_is_bad(__err) ((__err) & DSISR_BAD_FAULT_32S)
#endif
-#endif
/*
* For 600- and 800-family processors, the error_code parameter is DSISR
--
2.25.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v1 2/5] powerpc/fault: Unnest definition of page_fault_is_write() and page_fault_is_bad()
@ 2020-08-06 17:15 ` Christophe Leroy
0 siblings, 0 replies; 29+ messages in thread
From: Christophe Leroy @ 2020-08-06 17:15 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, npiggin
Cc: linuxppc-dev, linux-kernel
To make it more readable, separate page_fault_is_write() and page_fault_is_bad()
to avoir several levels of #ifdefs
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/mm/fault.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 2efa34d7e644..9ef9ee244f72 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -363,17 +363,19 @@ static void sanity_check_fault(bool is_write, bool is_user,
*/
#if (defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
#define page_fault_is_write(__err) ((__err) & ESR_DST)
-#define page_fault_is_bad(__err) (0)
#else
#define page_fault_is_write(__err) ((__err) & DSISR_ISSTORE)
-#if defined(CONFIG_PPC_8xx)
+#endif
+
+#if defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
+#define page_fault_is_bad(__err) (0)
+#elif defined(CONFIG_PPC_8xx)
#define page_fault_is_bad(__err) ((__err) & DSISR_NOEXEC_OR_G)
#elif defined(CONFIG_PPC64)
#define page_fault_is_bad(__err) ((__err) & DSISR_BAD_FAULT_64S)
#else
#define page_fault_is_bad(__err) ((__err) & DSISR_BAD_FAULT_32S)
#endif
-#endif
/*
* For 600- and 800-family processors, the error_code parameter is DSISR
--
2.25.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v1 3/5] powerpc/fault: Reorder tests in bad_kernel_fault()
2020-08-06 17:15 ` Christophe Leroy
@ 2020-08-06 17:15 ` Christophe Leroy
-1 siblings, 0 replies; 29+ messages in thread
From: Christophe Leroy @ 2020-08-06 17:15 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, npiggin
Cc: linux-kernel, linuxppc-dev
Check address earlier to simplify the following test.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/mm/fault.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 9ef9ee244f72..525e0c2b5406 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -210,17 +210,17 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
return true;
}
- if (!is_exec && address < TASK_SIZE && (error_code & DSISR_PROTFAULT) &&
+ // Kernel fault on kernel address is bad
+ if (address >= TASK_SIZE)
+ return true;
+
+ if (!is_exec && (error_code & DSISR_PROTFAULT) &&
!search_exception_tables(regs->nip)) {
pr_crit_ratelimited("Kernel attempted to access user page (%lx) - exploit attempt? (uid: %d)\n",
address,
from_kuid(&init_user_ns, current_uid()));
}
- // Kernel fault on kernel address is bad
- if (address >= TASK_SIZE)
- return true;
-
// Fault on user outside of certain regions (eg. copy_tofrom_user()) is bad
if (!search_exception_tables(regs->nip))
return true;
--
2.25.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v1 3/5] powerpc/fault: Reorder tests in bad_kernel_fault()
@ 2020-08-06 17:15 ` Christophe Leroy
0 siblings, 0 replies; 29+ messages in thread
From: Christophe Leroy @ 2020-08-06 17:15 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, npiggin
Cc: linuxppc-dev, linux-kernel
Check address earlier to simplify the following test.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/mm/fault.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 9ef9ee244f72..525e0c2b5406 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -210,17 +210,17 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
return true;
}
- if (!is_exec && address < TASK_SIZE && (error_code & DSISR_PROTFAULT) &&
+ // Kernel fault on kernel address is bad
+ if (address >= TASK_SIZE)
+ return true;
+
+ if (!is_exec && (error_code & DSISR_PROTFAULT) &&
!search_exception_tables(regs->nip)) {
pr_crit_ratelimited("Kernel attempted to access user page (%lx) - exploit attempt? (uid: %d)\n",
address,
from_kuid(&init_user_ns, current_uid()));
}
- // Kernel fault on kernel address is bad
- if (address >= TASK_SIZE)
- return true;
-
// Fault on user outside of certain regions (eg. copy_tofrom_user()) is bad
if (!search_exception_tables(regs->nip))
return true;
--
2.25.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v1 4/5] powerpc/fault: Avoid heavy search_exception_tables() verification
2020-08-06 17:15 ` Christophe Leroy
@ 2020-08-06 17:15 ` Christophe Leroy
-1 siblings, 0 replies; 29+ messages in thread
From: Christophe Leroy @ 2020-08-06 17:15 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, npiggin
Cc: linux-kernel, linuxppc-dev
search_exception_tables() is an heavy operation, we have to avoid it.
When KUAP is selected, we'll know the fault has been blocked by KUAP.
Otherwise, it behaves just as if the address was already in the TLBs
and no fault was generated.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/mm/fault.c | 20 +++++---------------
1 file changed, 5 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 525e0c2b5406..edde169ba3a6 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -214,24 +214,14 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
if (address >= TASK_SIZE)
return true;
- if (!is_exec && (error_code & DSISR_PROTFAULT) &&
- !search_exception_tables(regs->nip)) {
+ // Read/write fault blocked by KUAP is bad, it can never succeed.
+ if (bad_kuap_fault(regs, address, is_write)) {
pr_crit_ratelimited("Kernel attempted to access user page (%lx) - exploit attempt? (uid: %d)\n",
- address,
- from_kuid(&init_user_ns, current_uid()));
- }
-
- // Fault on user outside of certain regions (eg. copy_tofrom_user()) is bad
- if (!search_exception_tables(regs->nip))
- return true;
-
- // Read/write fault in a valid region (the exception table search passed
- // above), but blocked by KUAP is bad, it can never succeed.
- if (bad_kuap_fault(regs, address, is_write))
+ address, from_kuid(&init_user_ns, current_uid()));
return true;
+ }
- // What's left? Kernel fault on user in well defined regions (extable
- // matched), and allowed by KUAP in the faulting context.
+ // What's left? Kernel fault on user and allowed by KUAP in the faulting context.
return false;
}
--
2.25.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v1 4/5] powerpc/fault: Avoid heavy search_exception_tables() verification
@ 2020-08-06 17:15 ` Christophe Leroy
0 siblings, 0 replies; 29+ messages in thread
From: Christophe Leroy @ 2020-08-06 17:15 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, npiggin
Cc: linuxppc-dev, linux-kernel
search_exception_tables() is an heavy operation, we have to avoid it.
When KUAP is selected, we'll know the fault has been blocked by KUAP.
Otherwise, it behaves just as if the address was already in the TLBs
and no fault was generated.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/mm/fault.c | 20 +++++---------------
1 file changed, 5 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 525e0c2b5406..edde169ba3a6 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -214,24 +214,14 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
if (address >= TASK_SIZE)
return true;
- if (!is_exec && (error_code & DSISR_PROTFAULT) &&
- !search_exception_tables(regs->nip)) {
+ // Read/write fault blocked by KUAP is bad, it can never succeed.
+ if (bad_kuap_fault(regs, address, is_write)) {
pr_crit_ratelimited("Kernel attempted to access user page (%lx) - exploit attempt? (uid: %d)\n",
- address,
- from_kuid(&init_user_ns, current_uid()));
- }
-
- // Fault on user outside of certain regions (eg. copy_tofrom_user()) is bad
- if (!search_exception_tables(regs->nip))
- return true;
-
- // Read/write fault in a valid region (the exception table search passed
- // above), but blocked by KUAP is bad, it can never succeed.
- if (bad_kuap_fault(regs, address, is_write))
+ address, from_kuid(&init_user_ns, current_uid()));
return true;
+ }
- // What's left? Kernel fault on user in well defined regions (extable
- // matched), and allowed by KUAP in the faulting context.
+ // What's left? Kernel fault on user and allowed by KUAP in the faulting context.
return false;
}
--
2.25.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v1 5/5] powerpc/fault: Perform exception fixup in do_page_fault()
2020-08-06 17:15 ` Christophe Leroy
@ 2020-08-06 17:15 ` Christophe Leroy
-1 siblings, 0 replies; 29+ messages in thread
From: Christophe Leroy @ 2020-08-06 17:15 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, npiggin
Cc: linux-kernel, linuxppc-dev
Exception fixup doesn't require the heady full regs saving,
do it from do_page_fault() directly.
For that, split bad_page_fault() in two parts.
As bad_page_fault() can also be called from other places than
handle_page_fault(), it will still perform exception fixup and
fallback on __bad_page_fault().
handle_page_fault() directly calls __bad_page_fault() as the
exception fixup will now be done by do_page_fault()
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/kernel/entry_32.S | 2 +-
arch/powerpc/kernel/exceptions-64e.S | 2 +-
arch/powerpc/kernel/exceptions-64s.S | 2 +-
arch/powerpc/mm/fault.c | 33 ++++++++++++++++++++--------
4 files changed, 27 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
index f4d0af8e1136..c198786591f9 100644
--- a/arch/powerpc/kernel/entry_32.S
+++ b/arch/powerpc/kernel/entry_32.S
@@ -678,7 +678,7 @@ handle_page_fault:
mr r5,r3
addi r3,r1,STACK_FRAME_OVERHEAD
lwz r4,_DAR(r1)
- bl bad_page_fault
+ bl __bad_page_fault
b ret_from_except_full
#ifdef CONFIG_PPC_BOOK3S_32
diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
index d9ed79415100..dd9161ea5da8 100644
--- a/arch/powerpc/kernel/exceptions-64e.S
+++ b/arch/powerpc/kernel/exceptions-64e.S
@@ -1024,7 +1024,7 @@ storage_fault_common:
mr r5,r3
addi r3,r1,STACK_FRAME_OVERHEAD
ld r4,_DAR(r1)
- bl bad_page_fault
+ bl __bad_page_fault
b ret_from_except
/*
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index f7d748b88705..2cb3bcfb896d 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -3254,7 +3254,7 @@ handle_page_fault:
mr r5,r3
addi r3,r1,STACK_FRAME_OVERHEAD
ld r4,_DAR(r1)
- bl bad_page_fault
+ bl __bad_page_fault
b interrupt_return
/* We have a data breakpoint exception - handle it */
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index edde169ba3a6..bd6e397eb84a 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -542,10 +542,20 @@ NOKPROBE_SYMBOL(__do_page_fault);
int do_page_fault(struct pt_regs *regs, unsigned long address,
unsigned long error_code)
{
+ const struct exception_table_entry *entry;
enum ctx_state prev_state = exception_enter();
int rc = __do_page_fault(regs, address, error_code);
exception_exit(prev_state);
- return rc;
+ if (likely(!rc))
+ return 0;
+
+ entry = search_exception_tables(regs->nip);
+ if (unlikely(!entry))
+ return rc;
+
+ instruction_pointer_set(regs, extable_fixup(entry));
+
+ return 0;
}
NOKPROBE_SYMBOL(do_page_fault);
@@ -554,17 +564,10 @@ NOKPROBE_SYMBOL(do_page_fault);
* It is called from the DSI and ISI handlers in head.S and from some
* of the procedures in traps.c.
*/
-void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
+void __bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
{
- const struct exception_table_entry *entry;
int is_write = page_fault_is_write(regs->dsisr);
- /* Are we prepared to handle this fault? */
- if ((entry = search_exception_tables(regs->nip)) != NULL) {
- regs->nip = extable_fixup(entry);
- return;
- }
-
/* kernel has accessed a bad area */
switch (TRAP(regs)) {
@@ -598,3 +601,15 @@ void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
die("Kernel access of bad area", regs, sig);
}
+
+void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
+{
+ const struct exception_table_entry *entry;
+
+ /* Are we prepared to handle this fault? */
+ entry = search_exception_tables(instruction_pointer(regs));
+ if (entry)
+ instruction_pointer_set(regs, extable_fixup(entry));
+ else
+ __bad_page_fault(regs, address, sig);
+}
--
2.25.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v1 5/5] powerpc/fault: Perform exception fixup in do_page_fault()
@ 2020-08-06 17:15 ` Christophe Leroy
0 siblings, 0 replies; 29+ messages in thread
From: Christophe Leroy @ 2020-08-06 17:15 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, npiggin
Cc: linuxppc-dev, linux-kernel
Exception fixup doesn't require the heady full regs saving,
do it from do_page_fault() directly.
For that, split bad_page_fault() in two parts.
As bad_page_fault() can also be called from other places than
handle_page_fault(), it will still perform exception fixup and
fallback on __bad_page_fault().
handle_page_fault() directly calls __bad_page_fault() as the
exception fixup will now be done by do_page_fault()
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/kernel/entry_32.S | 2 +-
arch/powerpc/kernel/exceptions-64e.S | 2 +-
arch/powerpc/kernel/exceptions-64s.S | 2 +-
arch/powerpc/mm/fault.c | 33 ++++++++++++++++++++--------
4 files changed, 27 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
index f4d0af8e1136..c198786591f9 100644
--- a/arch/powerpc/kernel/entry_32.S
+++ b/arch/powerpc/kernel/entry_32.S
@@ -678,7 +678,7 @@ handle_page_fault:
mr r5,r3
addi r3,r1,STACK_FRAME_OVERHEAD
lwz r4,_DAR(r1)
- bl bad_page_fault
+ bl __bad_page_fault
b ret_from_except_full
#ifdef CONFIG_PPC_BOOK3S_32
diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
index d9ed79415100..dd9161ea5da8 100644
--- a/arch/powerpc/kernel/exceptions-64e.S
+++ b/arch/powerpc/kernel/exceptions-64e.S
@@ -1024,7 +1024,7 @@ storage_fault_common:
mr r5,r3
addi r3,r1,STACK_FRAME_OVERHEAD
ld r4,_DAR(r1)
- bl bad_page_fault
+ bl __bad_page_fault
b ret_from_except
/*
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index f7d748b88705..2cb3bcfb896d 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -3254,7 +3254,7 @@ handle_page_fault:
mr r5,r3
addi r3,r1,STACK_FRAME_OVERHEAD
ld r4,_DAR(r1)
- bl bad_page_fault
+ bl __bad_page_fault
b interrupt_return
/* We have a data breakpoint exception - handle it */
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index edde169ba3a6..bd6e397eb84a 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -542,10 +542,20 @@ NOKPROBE_SYMBOL(__do_page_fault);
int do_page_fault(struct pt_regs *regs, unsigned long address,
unsigned long error_code)
{
+ const struct exception_table_entry *entry;
enum ctx_state prev_state = exception_enter();
int rc = __do_page_fault(regs, address, error_code);
exception_exit(prev_state);
- return rc;
+ if (likely(!rc))
+ return 0;
+
+ entry = search_exception_tables(regs->nip);
+ if (unlikely(!entry))
+ return rc;
+
+ instruction_pointer_set(regs, extable_fixup(entry));
+
+ return 0;
}
NOKPROBE_SYMBOL(do_page_fault);
@@ -554,17 +564,10 @@ NOKPROBE_SYMBOL(do_page_fault);
* It is called from the DSI and ISI handlers in head.S and from some
* of the procedures in traps.c.
*/
-void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
+void __bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
{
- const struct exception_table_entry *entry;
int is_write = page_fault_is_write(regs->dsisr);
- /* Are we prepared to handle this fault? */
- if ((entry = search_exception_tables(regs->nip)) != NULL) {
- regs->nip = extable_fixup(entry);
- return;
- }
-
/* kernel has accessed a bad area */
switch (TRAP(regs)) {
@@ -598,3 +601,15 @@ void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
die("Kernel access of bad area", regs, sig);
}
+
+void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
+{
+ const struct exception_table_entry *entry;
+
+ /* Are we prepared to handle this fault? */
+ entry = search_exception_tables(instruction_pointer(regs));
+ if (entry)
+ instruction_pointer_set(regs, extable_fixup(entry));
+ else
+ __bad_page_fault(regs, address, sig);
+}
--
2.25.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v1 5/5] powerpc/fault: Perform exception fixup in do_page_fault()
2020-08-06 17:15 ` Christophe Leroy
(?)
@ 2020-08-06 21:07 ` kernel test robot
-1 siblings, 0 replies; 29+ messages in thread
From: kernel test robot @ 2020-08-06 21:07 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, npiggin
Cc: kbuild-all, linux-kernel, linuxppc-dev
[-- Attachment #1: Type: text/plain, Size: 3059 bytes --]
Hi Christophe,
I love your patch! Perhaps something to improve:
[auto build test WARNING on powerpc/next]
[also build test WARNING on v5.8 next-20200806]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-mm-sanity_check_fault-should-work-for-all-not-only-BOOK3S/20200807-012433
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-allyesconfig (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> arch/powerpc/mm/fault.c:567:6: warning: no previous prototype for '__bad_page_fault' [-Wmissing-prototypes]
567 | void __bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
| ^~~~~~~~~~~~~~~~
vim +/__bad_page_fault +567 arch/powerpc/mm/fault.c
561
562 /*
563 * bad_page_fault is called when we have a bad access from the kernel.
564 * It is called from the DSI and ISI handlers in head.S and from some
565 * of the procedures in traps.c.
566 */
> 567 void __bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
568 {
569 int is_write = page_fault_is_write(regs->dsisr);
570
571 /* kernel has accessed a bad area */
572
573 switch (TRAP(regs)) {
574 case 0x300:
575 case 0x380:
576 case 0xe00:
577 pr_alert("BUG: %s on %s at 0x%08lx\n",
578 regs->dar < PAGE_SIZE ? "Kernel NULL pointer dereference" :
579 "Unable to handle kernel data access",
580 is_write ? "write" : "read", regs->dar);
581 break;
582 case 0x400:
583 case 0x480:
584 pr_alert("BUG: Unable to handle kernel instruction fetch%s",
585 regs->nip < PAGE_SIZE ? " (NULL pointer?)\n" : "\n");
586 break;
587 case 0x600:
588 pr_alert("BUG: Unable to handle kernel unaligned access at 0x%08lx\n",
589 regs->dar);
590 break;
591 default:
592 pr_alert("BUG: Unable to handle unknown paging fault at 0x%08lx\n",
593 regs->dar);
594 break;
595 }
596 printk(KERN_ALERT "Faulting instruction address: 0x%08lx\n",
597 regs->nip);
598
599 if (task_stack_end_corrupted(current))
600 printk(KERN_ALERT "Thread overran stack, or stack corrupted\n");
601
602 die("Kernel access of bad area", regs, sig);
603 }
604
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 69763 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v1 5/5] powerpc/fault: Perform exception fixup in do_page_fault()
@ 2020-08-06 21:07 ` kernel test robot
0 siblings, 0 replies; 29+ messages in thread
From: kernel test robot @ 2020-08-06 21:07 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, npiggin
Cc: linuxppc-dev, kbuild-all, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 3059 bytes --]
Hi Christophe,
I love your patch! Perhaps something to improve:
[auto build test WARNING on powerpc/next]
[also build test WARNING on v5.8 next-20200806]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-mm-sanity_check_fault-should-work-for-all-not-only-BOOK3S/20200807-012433
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-allyesconfig (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> arch/powerpc/mm/fault.c:567:6: warning: no previous prototype for '__bad_page_fault' [-Wmissing-prototypes]
567 | void __bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
| ^~~~~~~~~~~~~~~~
vim +/__bad_page_fault +567 arch/powerpc/mm/fault.c
561
562 /*
563 * bad_page_fault is called when we have a bad access from the kernel.
564 * It is called from the DSI and ISI handlers in head.S and from some
565 * of the procedures in traps.c.
566 */
> 567 void __bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
568 {
569 int is_write = page_fault_is_write(regs->dsisr);
570
571 /* kernel has accessed a bad area */
572
573 switch (TRAP(regs)) {
574 case 0x300:
575 case 0x380:
576 case 0xe00:
577 pr_alert("BUG: %s on %s at 0x%08lx\n",
578 regs->dar < PAGE_SIZE ? "Kernel NULL pointer dereference" :
579 "Unable to handle kernel data access",
580 is_write ? "write" : "read", regs->dar);
581 break;
582 case 0x400:
583 case 0x480:
584 pr_alert("BUG: Unable to handle kernel instruction fetch%s",
585 regs->nip < PAGE_SIZE ? " (NULL pointer?)\n" : "\n");
586 break;
587 case 0x600:
588 pr_alert("BUG: Unable to handle kernel unaligned access at 0x%08lx\n",
589 regs->dar);
590 break;
591 default:
592 pr_alert("BUG: Unable to handle unknown paging fault at 0x%08lx\n",
593 regs->dar);
594 break;
595 }
596 printk(KERN_ALERT "Faulting instruction address: 0x%08lx\n",
597 regs->nip);
598
599 if (task_stack_end_corrupted(current))
600 printk(KERN_ALERT "Thread overran stack, or stack corrupted\n");
601
602 die("Kernel access of bad area", regs, sig);
603 }
604
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 69763 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v1 5/5] powerpc/fault: Perform exception fixup in do_page_fault()
@ 2020-08-06 21:07 ` kernel test robot
0 siblings, 0 replies; 29+ messages in thread
From: kernel test robot @ 2020-08-06 21:07 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 3137 bytes --]
Hi Christophe,
I love your patch! Perhaps something to improve:
[auto build test WARNING on powerpc/next]
[also build test WARNING on v5.8 next-20200806]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-mm-sanity_check_fault-should-work-for-all-not-only-BOOK3S/20200807-012433
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-allyesconfig (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> arch/powerpc/mm/fault.c:567:6: warning: no previous prototype for '__bad_page_fault' [-Wmissing-prototypes]
567 | void __bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
| ^~~~~~~~~~~~~~~~
vim +/__bad_page_fault +567 arch/powerpc/mm/fault.c
561
562 /*
563 * bad_page_fault is called when we have a bad access from the kernel.
564 * It is called from the DSI and ISI handlers in head.S and from some
565 * of the procedures in traps.c.
566 */
> 567 void __bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
568 {
569 int is_write = page_fault_is_write(regs->dsisr);
570
571 /* kernel has accessed a bad area */
572
573 switch (TRAP(regs)) {
574 case 0x300:
575 case 0x380:
576 case 0xe00:
577 pr_alert("BUG: %s on %s at 0x%08lx\n",
578 regs->dar < PAGE_SIZE ? "Kernel NULL pointer dereference" :
579 "Unable to handle kernel data access",
580 is_write ? "write" : "read", regs->dar);
581 break;
582 case 0x400:
583 case 0x480:
584 pr_alert("BUG: Unable to handle kernel instruction fetch%s",
585 regs->nip < PAGE_SIZE ? " (NULL pointer?)\n" : "\n");
586 break;
587 case 0x600:
588 pr_alert("BUG: Unable to handle kernel unaligned access@0x%08lx\n",
589 regs->dar);
590 break;
591 default:
592 pr_alert("BUG: Unable to handle unknown paging fault at 0x%08lx\n",
593 regs->dar);
594 break;
595 }
596 printk(KERN_ALERT "Faulting instruction address: 0x%08lx\n",
597 regs->nip);
598
599 if (task_stack_end_corrupted(current))
600 printk(KERN_ALERT "Thread overran stack, or stack corrupted\n");
601
602 die("Kernel access of bad area", regs, sig);
603 }
604
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 69763 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v1 1/5] powerpc/mm: sanity_check_fault() should work for all, not only BOOK3S
2020-08-06 17:15 ` Christophe Leroy
@ 2020-09-08 8:43 ` Nicholas Piggin
-1 siblings, 0 replies; 29+ messages in thread
From: Nicholas Piggin @ 2020-09-08 8:43 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linux-kernel, linuxppc-dev
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 <christophe.leroy@csgroup.eu>
> ---
> 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 <npiggin@gmail.com>
> +
> /*
> * 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
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v1 1/5] powerpc/mm: sanity_check_fault() should work for all, not only BOOK3S
@ 2020-09-08 8:43 ` Nicholas Piggin
0 siblings, 0 replies; 29+ messages in thread
From: Nicholas Piggin @ 2020-09-08 8:43 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
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 <christophe.leroy@csgroup.eu>
> ---
> 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 <npiggin@gmail.com>
> +
> /*
> * 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
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v1 2/5] powerpc/fault: Unnest definition of page_fault_is_write() and page_fault_is_bad()
2020-08-06 17:15 ` Christophe Leroy
@ 2020-09-08 8:44 ` Nicholas Piggin
-1 siblings, 0 replies; 29+ messages in thread
From: Nicholas Piggin @ 2020-09-08 8:44 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linux-kernel, linuxppc-dev
Excerpts from Christophe Leroy's message of August 7, 2020 3:15 am:
> To make it more readable, separate page_fault_is_write() and page_fault_is_bad()
> to avoir several levels of #ifdefs
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/mm/fault.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 2efa34d7e644..9ef9ee244f72 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -363,17 +363,19 @@ static void sanity_check_fault(bool is_write, bool is_user,
> */
> #if (defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
> #define page_fault_is_write(__err) ((__err) & ESR_DST)
> -#define page_fault_is_bad(__err) (0)
> #else
> #define page_fault_is_write(__err) ((__err) & DSISR_ISSTORE)
> -#if defined(CONFIG_PPC_8xx)
> +#endif
> +
> +#if defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
> +#define page_fault_is_bad(__err) (0)
> +#elif defined(CONFIG_PPC_8xx)
> #define page_fault_is_bad(__err) ((__err) & DSISR_NOEXEC_OR_G)
> #elif defined(CONFIG_PPC64)
> #define page_fault_is_bad(__err) ((__err) & DSISR_BAD_FAULT_64S)
> #else
> #define page_fault_is_bad(__err) ((__err) & DSISR_BAD_FAULT_32S)
> #endif
> -#endif
>
> /*
> * For 600- and 800-family processors, the error_code parameter is DSISR
> --
> 2.25.0
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v1 2/5] powerpc/fault: Unnest definition of page_fault_is_write() and page_fault_is_bad()
@ 2020-09-08 8:44 ` Nicholas Piggin
0 siblings, 0 replies; 29+ messages in thread
From: Nicholas Piggin @ 2020-09-08 8:44 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
Excerpts from Christophe Leroy's message of August 7, 2020 3:15 am:
> To make it more readable, separate page_fault_is_write() and page_fault_is_bad()
> to avoir several levels of #ifdefs
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/mm/fault.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 2efa34d7e644..9ef9ee244f72 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -363,17 +363,19 @@ static void sanity_check_fault(bool is_write, bool is_user,
> */
> #if (defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
> #define page_fault_is_write(__err) ((__err) & ESR_DST)
> -#define page_fault_is_bad(__err) (0)
> #else
> #define page_fault_is_write(__err) ((__err) & DSISR_ISSTORE)
> -#if defined(CONFIG_PPC_8xx)
> +#endif
> +
> +#if defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
> +#define page_fault_is_bad(__err) (0)
> +#elif defined(CONFIG_PPC_8xx)
> #define page_fault_is_bad(__err) ((__err) & DSISR_NOEXEC_OR_G)
> #elif defined(CONFIG_PPC64)
> #define page_fault_is_bad(__err) ((__err) & DSISR_BAD_FAULT_64S)
> #else
> #define page_fault_is_bad(__err) ((__err) & DSISR_BAD_FAULT_32S)
> #endif
> -#endif
>
> /*
> * For 600- and 800-family processors, the error_code parameter is DSISR
> --
> 2.25.0
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v1 3/5] powerpc/fault: Reorder tests in bad_kernel_fault()
2020-08-06 17:15 ` Christophe Leroy
@ 2020-09-08 8:46 ` Nicholas Piggin
-1 siblings, 0 replies; 29+ messages in thread
From: Nicholas Piggin @ 2020-09-08 8:46 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linux-kernel, linuxppc-dev
Excerpts from Christophe Leroy's message of August 7, 2020 3:15 am:
> Check address earlier to simplify the following test.
Good logic reduction.
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/mm/fault.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 9ef9ee244f72..525e0c2b5406 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -210,17 +210,17 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
> return true;
> }
>
> - if (!is_exec && address < TASK_SIZE && (error_code & DSISR_PROTFAULT) &&
> + // Kernel fault on kernel address is bad
> + if (address >= TASK_SIZE)
> + return true;
> +
> + if (!is_exec && (error_code & DSISR_PROTFAULT) &&
> !search_exception_tables(regs->nip)) {
> pr_crit_ratelimited("Kernel attempted to access user page (%lx) - exploit attempt? (uid: %d)\n",
> address,
> from_kuid(&init_user_ns, current_uid()));
> }
>
> - // Kernel fault on kernel address is bad
> - if (address >= TASK_SIZE)
> - return true;
> -
> // Fault on user outside of certain regions (eg. copy_tofrom_user()) is bad
> if (!search_exception_tables(regs->nip))
> return true;
> --
> 2.25.0
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v1 3/5] powerpc/fault: Reorder tests in bad_kernel_fault()
@ 2020-09-08 8:46 ` Nicholas Piggin
0 siblings, 0 replies; 29+ messages in thread
From: Nicholas Piggin @ 2020-09-08 8:46 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
Excerpts from Christophe Leroy's message of August 7, 2020 3:15 am:
> Check address earlier to simplify the following test.
Good logic reduction.
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/mm/fault.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 9ef9ee244f72..525e0c2b5406 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -210,17 +210,17 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
> return true;
> }
>
> - if (!is_exec && address < TASK_SIZE && (error_code & DSISR_PROTFAULT) &&
> + // Kernel fault on kernel address is bad
> + if (address >= TASK_SIZE)
> + return true;
> +
> + if (!is_exec && (error_code & DSISR_PROTFAULT) &&
> !search_exception_tables(regs->nip)) {
> pr_crit_ratelimited("Kernel attempted to access user page (%lx) - exploit attempt? (uid: %d)\n",
> address,
> from_kuid(&init_user_ns, current_uid()));
> }
>
> - // Kernel fault on kernel address is bad
> - if (address >= TASK_SIZE)
> - return true;
> -
> // Fault on user outside of certain regions (eg. copy_tofrom_user()) is bad
> if (!search_exception_tables(regs->nip))
> return true;
> --
> 2.25.0
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v1 4/5] powerpc/fault: Avoid heavy search_exception_tables() verification
2020-08-06 17:15 ` Christophe Leroy
@ 2020-09-08 8:54 ` Nicholas Piggin
-1 siblings, 0 replies; 29+ messages in thread
From: Nicholas Piggin @ 2020-09-08 8:54 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linux-kernel, linuxppc-dev
Excerpts from Christophe Leroy's message of August 7, 2020 3:15 am:
> search_exception_tables() is an heavy operation, we have to avoid it.
> When KUAP is selected, we'll know the fault has been blocked by KUAP.
> Otherwise, it behaves just as if the address was already in the TLBs
> and no fault was generated.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Sorry I missed reviewing this. Yes, we discussed this and decided
that it's not effective I think (and KUAP solves it properly).
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/mm/fault.c | 20 +++++---------------
> 1 file changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 525e0c2b5406..edde169ba3a6 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -214,24 +214,14 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
> if (address >= TASK_SIZE)
> return true;
>
> - if (!is_exec && (error_code & DSISR_PROTFAULT) &&
> - !search_exception_tables(regs->nip)) {
> + // Read/write fault blocked by KUAP is bad, it can never succeed.
> + if (bad_kuap_fault(regs, address, is_write)) {
> pr_crit_ratelimited("Kernel attempted to access user page (%lx) - exploit attempt? (uid: %d)\n",
> - address,
> - from_kuid(&init_user_ns, current_uid()));
> - }
> -
> - // Fault on user outside of certain regions (eg. copy_tofrom_user()) is bad
> - if (!search_exception_tables(regs->nip))
> - return true;
> -
> - // Read/write fault in a valid region (the exception table search passed
> - // above), but blocked by KUAP is bad, it can never succeed.
> - if (bad_kuap_fault(regs, address, is_write))
> + address, from_kuid(&init_user_ns, current_uid()));
> return true;
> + }
>
> - // What's left? Kernel fault on user in well defined regions (extable
> - // matched), and allowed by KUAP in the faulting context.
> + // What's left? Kernel fault on user and allowed by KUAP in the faulting context.
> return false;
> }
>
> --
> 2.25.0
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v1 4/5] powerpc/fault: Avoid heavy search_exception_tables() verification
@ 2020-09-08 8:54 ` Nicholas Piggin
0 siblings, 0 replies; 29+ messages in thread
From: Nicholas Piggin @ 2020-09-08 8:54 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
Excerpts from Christophe Leroy's message of August 7, 2020 3:15 am:
> search_exception_tables() is an heavy operation, we have to avoid it.
> When KUAP is selected, we'll know the fault has been blocked by KUAP.
> Otherwise, it behaves just as if the address was already in the TLBs
> and no fault was generated.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Sorry I missed reviewing this. Yes, we discussed this and decided
that it's not effective I think (and KUAP solves it properly).
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/mm/fault.c | 20 +++++---------------
> 1 file changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 525e0c2b5406..edde169ba3a6 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -214,24 +214,14 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
> if (address >= TASK_SIZE)
> return true;
>
> - if (!is_exec && (error_code & DSISR_PROTFAULT) &&
> - !search_exception_tables(regs->nip)) {
> + // Read/write fault blocked by KUAP is bad, it can never succeed.
> + if (bad_kuap_fault(regs, address, is_write)) {
> pr_crit_ratelimited("Kernel attempted to access user page (%lx) - exploit attempt? (uid: %d)\n",
> - address,
> - from_kuid(&init_user_ns, current_uid()));
> - }
> -
> - // Fault on user outside of certain regions (eg. copy_tofrom_user()) is bad
> - if (!search_exception_tables(regs->nip))
> - return true;
> -
> - // Read/write fault in a valid region (the exception table search passed
> - // above), but blocked by KUAP is bad, it can never succeed.
> - if (bad_kuap_fault(regs, address, is_write))
> + address, from_kuid(&init_user_ns, current_uid()));
> return true;
> + }
>
> - // What's left? Kernel fault on user in well defined regions (extable
> - // matched), and allowed by KUAP in the faulting context.
> + // What's left? Kernel fault on user and allowed by KUAP in the faulting context.
> return false;
> }
>
> --
> 2.25.0
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v1 1/5] powerpc/mm: sanity_check_fault() should work for all, not only BOOK3S
2020-09-08 8:43 ` Nicholas Piggin
@ 2020-09-08 8:56 ` Christophe Leroy
-1 siblings, 0 replies; 29+ messages in thread
From: Christophe Leroy @ 2020-09-08 8:56 UTC (permalink / raw)
To: Nicholas Piggin, Benjamin Herrenschmidt, Michael Ellerman,
Paul Mackerras
Cc: linux-kernel, linuxppc-dev
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 <christophe.leroy@csgroup.eu>
>> ---
>> 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 0xc0000000 to 0xffffffff
But we could skip the top page entirely, anyway it is never mapped.
>
> Anyway for your patch
>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
Thanks
Christophe
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v1 1/5] powerpc/mm: sanity_check_fault() should work for all, not only BOOK3S
@ 2020-09-08 8:56 ` Christophe Leroy
0 siblings, 0 replies; 29+ messages in thread
From: Christophe Leroy @ 2020-09-08 8:56 UTC (permalink / raw)
To: Nicholas Piggin, Benjamin Herrenschmidt, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
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 <christophe.leroy@csgroup.eu>
>> ---
>> 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 0xc0000000 to 0xffffffff
But we could skip the top page entirely, anyway it is never mapped.
>
> Anyway for your patch
>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
Thanks
Christophe
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v1 5/5] powerpc/fault: Perform exception fixup in do_page_fault()
2020-08-06 17:15 ` Christophe Leroy
@ 2020-09-08 9:13 ` Nicholas Piggin
-1 siblings, 0 replies; 29+ messages in thread
From: Nicholas Piggin @ 2020-09-08 9:13 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linux-kernel, linuxppc-dev
Excerpts from Christophe Leroy's message of August 7, 2020 3:15 am:
> Exception fixup doesn't require the heady full regs saving,
heavy
> do it from do_page_fault() directly.
>
> For that, split bad_page_fault() in two parts.
>
> As bad_page_fault() can also be called from other places than
> handle_page_fault(), it will still perform exception fixup and
> fallback on __bad_page_fault().
>
> handle_page_fault() directly calls __bad_page_fault() as the
> exception fixup will now be done by do_page_fault()
Looks good. We can probably get rid of bad_page_fault completely after
this too.
Hmm, the alignment exception might(?) hit user copies if the user points
it to CI memory. Then you could race and the memory gets unmapped. In
that case the exception table check might be better to be explicit there
with comments.
The first call in do_hash_fault is not required (copy user will never
be in nmi context). The second one and the one in slb_fault could be
made explicit too. Anyway for now this is fine.
Thanks,
Nick
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/kernel/entry_32.S | 2 +-
> arch/powerpc/kernel/exceptions-64e.S | 2 +-
> arch/powerpc/kernel/exceptions-64s.S | 2 +-
> arch/powerpc/mm/fault.c | 33 ++++++++++++++++++++--------
> 4 files changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
> index f4d0af8e1136..c198786591f9 100644
> --- a/arch/powerpc/kernel/entry_32.S
> +++ b/arch/powerpc/kernel/entry_32.S
> @@ -678,7 +678,7 @@ handle_page_fault:
> mr r5,r3
> addi r3,r1,STACK_FRAME_OVERHEAD
> lwz r4,_DAR(r1)
> - bl bad_page_fault
> + bl __bad_page_fault
> b ret_from_except_full
>
> #ifdef CONFIG_PPC_BOOK3S_32
> diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
> index d9ed79415100..dd9161ea5da8 100644
> --- a/arch/powerpc/kernel/exceptions-64e.S
> +++ b/arch/powerpc/kernel/exceptions-64e.S
> @@ -1024,7 +1024,7 @@ storage_fault_common:
> mr r5,r3
> addi r3,r1,STACK_FRAME_OVERHEAD
> ld r4,_DAR(r1)
> - bl bad_page_fault
> + bl __bad_page_fault
> b ret_from_except
>
> /*
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index f7d748b88705..2cb3bcfb896d 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -3254,7 +3254,7 @@ handle_page_fault:
> mr r5,r3
> addi r3,r1,STACK_FRAME_OVERHEAD
> ld r4,_DAR(r1)
> - bl bad_page_fault
> + bl __bad_page_fault
> b interrupt_return
>
> /* We have a data breakpoint exception - handle it */
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index edde169ba3a6..bd6e397eb84a 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -542,10 +542,20 @@ NOKPROBE_SYMBOL(__do_page_fault);
> int do_page_fault(struct pt_regs *regs, unsigned long address,
> unsigned long error_code)
> {
> + const struct exception_table_entry *entry;
> enum ctx_state prev_state = exception_enter();
> int rc = __do_page_fault(regs, address, error_code);
> exception_exit(prev_state);
> - return rc;
> + if (likely(!rc))
> + return 0;
> +
> + entry = search_exception_tables(regs->nip);
> + if (unlikely(!entry))
> + return rc;
> +
> + instruction_pointer_set(regs, extable_fixup(entry));
> +
> + return 0;
> }
> NOKPROBE_SYMBOL(do_page_fault);
>
> @@ -554,17 +564,10 @@ NOKPROBE_SYMBOL(do_page_fault);
> * It is called from the DSI and ISI handlers in head.S and from some
> * of the procedures in traps.c.
> */
> -void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
> +void __bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
> {
> - const struct exception_table_entry *entry;
> int is_write = page_fault_is_write(regs->dsisr);
>
> - /* Are we prepared to handle this fault? */
> - if ((entry = search_exception_tables(regs->nip)) != NULL) {
> - regs->nip = extable_fixup(entry);
> - return;
> - }
> -
> /* kernel has accessed a bad area */
>
> switch (TRAP(regs)) {
> @@ -598,3 +601,15 @@ void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
>
> die("Kernel access of bad area", regs, sig);
> }
> +
> +void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
> +{
> + const struct exception_table_entry *entry;
> +
> + /* Are we prepared to handle this fault? */
> + entry = search_exception_tables(instruction_pointer(regs));
> + if (entry)
> + instruction_pointer_set(regs, extable_fixup(entry));
> + else
> + __bad_page_fault(regs, address, sig);
> +}
> --
> 2.25.0
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v1 5/5] powerpc/fault: Perform exception fixup in do_page_fault()
@ 2020-09-08 9:13 ` Nicholas Piggin
0 siblings, 0 replies; 29+ messages in thread
From: Nicholas Piggin @ 2020-09-08 9:13 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
Excerpts from Christophe Leroy's message of August 7, 2020 3:15 am:
> Exception fixup doesn't require the heady full regs saving,
heavy
> do it from do_page_fault() directly.
>
> For that, split bad_page_fault() in two parts.
>
> As bad_page_fault() can also be called from other places than
> handle_page_fault(), it will still perform exception fixup and
> fallback on __bad_page_fault().
>
> handle_page_fault() directly calls __bad_page_fault() as the
> exception fixup will now be done by do_page_fault()
Looks good. We can probably get rid of bad_page_fault completely after
this too.
Hmm, the alignment exception might(?) hit user copies if the user points
it to CI memory. Then you could race and the memory gets unmapped. In
that case the exception table check might be better to be explicit there
with comments.
The first call in do_hash_fault is not required (copy user will never
be in nmi context). The second one and the one in slb_fault could be
made explicit too. Anyway for now this is fine.
Thanks,
Nick
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/kernel/entry_32.S | 2 +-
> arch/powerpc/kernel/exceptions-64e.S | 2 +-
> arch/powerpc/kernel/exceptions-64s.S | 2 +-
> arch/powerpc/mm/fault.c | 33 ++++++++++++++++++++--------
> 4 files changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
> index f4d0af8e1136..c198786591f9 100644
> --- a/arch/powerpc/kernel/entry_32.S
> +++ b/arch/powerpc/kernel/entry_32.S
> @@ -678,7 +678,7 @@ handle_page_fault:
> mr r5,r3
> addi r3,r1,STACK_FRAME_OVERHEAD
> lwz r4,_DAR(r1)
> - bl bad_page_fault
> + bl __bad_page_fault
> b ret_from_except_full
>
> #ifdef CONFIG_PPC_BOOK3S_32
> diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
> index d9ed79415100..dd9161ea5da8 100644
> --- a/arch/powerpc/kernel/exceptions-64e.S
> +++ b/arch/powerpc/kernel/exceptions-64e.S
> @@ -1024,7 +1024,7 @@ storage_fault_common:
> mr r5,r3
> addi r3,r1,STACK_FRAME_OVERHEAD
> ld r4,_DAR(r1)
> - bl bad_page_fault
> + bl __bad_page_fault
> b ret_from_except
>
> /*
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index f7d748b88705..2cb3bcfb896d 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -3254,7 +3254,7 @@ handle_page_fault:
> mr r5,r3
> addi r3,r1,STACK_FRAME_OVERHEAD
> ld r4,_DAR(r1)
> - bl bad_page_fault
> + bl __bad_page_fault
> b interrupt_return
>
> /* We have a data breakpoint exception - handle it */
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index edde169ba3a6..bd6e397eb84a 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -542,10 +542,20 @@ NOKPROBE_SYMBOL(__do_page_fault);
> int do_page_fault(struct pt_regs *regs, unsigned long address,
> unsigned long error_code)
> {
> + const struct exception_table_entry *entry;
> enum ctx_state prev_state = exception_enter();
> int rc = __do_page_fault(regs, address, error_code);
> exception_exit(prev_state);
> - return rc;
> + if (likely(!rc))
> + return 0;
> +
> + entry = search_exception_tables(regs->nip);
> + if (unlikely(!entry))
> + return rc;
> +
> + instruction_pointer_set(regs, extable_fixup(entry));
> +
> + return 0;
> }
> NOKPROBE_SYMBOL(do_page_fault);
>
> @@ -554,17 +564,10 @@ NOKPROBE_SYMBOL(do_page_fault);
> * It is called from the DSI and ISI handlers in head.S and from some
> * of the procedures in traps.c.
> */
> -void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
> +void __bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
> {
> - const struct exception_table_entry *entry;
> int is_write = page_fault_is_write(regs->dsisr);
>
> - /* Are we prepared to handle this fault? */
> - if ((entry = search_exception_tables(regs->nip)) != NULL) {
> - regs->nip = extable_fixup(entry);
> - return;
> - }
> -
> /* kernel has accessed a bad area */
>
> switch (TRAP(regs)) {
> @@ -598,3 +601,15 @@ void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
>
> die("Kernel access of bad area", regs, sig);
> }
> +
> +void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
> +{
> + const struct exception_table_entry *entry;
> +
> + /* Are we prepared to handle this fault? */
> + entry = search_exception_tables(instruction_pointer(regs));
> + if (entry)
> + instruction_pointer_set(regs, extable_fixup(entry));
> + else
> + __bad_page_fault(regs, address, sig);
> +}
> --
> 2.25.0
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v1 4/5] powerpc/fault: Avoid heavy search_exception_tables() verification
2020-08-06 17:15 ` Christophe Leroy
(?)
(?)
@ 2020-09-09 6:04 ` Aneesh Kumar K.V
2020-09-09 6:20 ` Christophe Leroy
-1 siblings, 1 reply; 29+ messages in thread
From: Aneesh Kumar K.V @ 2020-09-09 6:04 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, npiggin
Cc: linuxppc-dev, linux-kernel
Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> search_exception_tables() is an heavy operation, we have to avoid it.
> When KUAP is selected, we'll know the fault has been blocked by KUAP.
> Otherwise, it behaves just as if the address was already in the TLBs
> and no fault was generated.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/mm/fault.c | 20 +++++---------------
> 1 file changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 525e0c2b5406..edde169ba3a6 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -214,24 +214,14 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
> if (address >= TASK_SIZE)
> return true;
>
> - if (!is_exec && (error_code & DSISR_PROTFAULT) &&
> - !search_exception_tables(regs->nip)) {
> + // Read/write fault blocked by KUAP is bad, it can never succeed.
> + if (bad_kuap_fault(regs, address, is_write)) {
> pr_crit_ratelimited("Kernel attempted to access user page (%lx) - exploit attempt? (uid: %d)\n",
> - address,
> - from_kuid(&init_user_ns, current_uid()));
> - }
> -
> - // Fault on user outside of certain regions (eg. copy_tofrom_user()) is bad
> - if (!search_exception_tables(regs->nip))
> - return true;
We still need to keep this ? Without that we detect the lack of
exception tables pretty late.
> -
> - // Read/write fault in a valid region (the exception table search passed
> - // above), but blocked by KUAP is bad, it can never succeed.
> - if (bad_kuap_fault(regs, address, is_write))
> + address, from_kuid(&init_user_ns, current_uid()));
> return true;
> + }
>
> - // What's left? Kernel fault on user in well defined regions (extable
> - // matched), and allowed by KUAP in the faulting context.
> + // What's left? Kernel fault on user and allowed by KUAP in the faulting context.
> return false;
> }
>
> --
> 2.25.0
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v1 4/5] powerpc/fault: Avoid heavy search_exception_tables() verification
2020-09-09 6:04 ` Aneesh Kumar K.V
@ 2020-09-09 6:20 ` Christophe Leroy
2020-09-14 2:23 ` Nicholas Piggin
0 siblings, 1 reply; 29+ messages in thread
From: Christophe Leroy @ 2020-09-09 6:20 UTC (permalink / raw)
To: Aneesh Kumar K.V, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, npiggin
Cc: linuxppc-dev, linux-kernel
Le 09/09/2020 à 08:04, Aneesh Kumar K.V a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>
>> search_exception_tables() is an heavy operation, we have to avoid it.
>> When KUAP is selected, we'll know the fault has been blocked by KUAP.
>> Otherwise, it behaves just as if the address was already in the TLBs
>> and no fault was generated.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> arch/powerpc/mm/fault.c | 20 +++++---------------
>> 1 file changed, 5 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>> index 525e0c2b5406..edde169ba3a6 100644
>> --- a/arch/powerpc/mm/fault.c
>> +++ b/arch/powerpc/mm/fault.c
>> @@ -214,24 +214,14 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
>> if (address >= TASK_SIZE)
>> return true;
>>
>> - if (!is_exec && (error_code & DSISR_PROTFAULT) &&
>> - !search_exception_tables(regs->nip)) {
>> + // Read/write fault blocked by KUAP is bad, it can never succeed.
>> + if (bad_kuap_fault(regs, address, is_write)) {
>> pr_crit_ratelimited("Kernel attempted to access user page (%lx) - exploit attempt? (uid: %d)\n",
>> - address,
>> - from_kuid(&init_user_ns, current_uid()));
>> - }
>> -
>> - // Fault on user outside of certain regions (eg. copy_tofrom_user()) is bad
>> - if (!search_exception_tables(regs->nip))
>> - return true;
>
> We still need to keep this ? Without that we detect the lack of
> exception tables pretty late.
Is that a problem at all to detect the lack of exception tables late ?
That case is very unlikely and will lead to failure anyway. So, is it
worth impacting performance of the likely case which will always have an
exception table and where we expect the exception to run as fast as
possible ?
The other architectures I have looked at (arm64 and x86) only have the
exception table search together with the down_read_trylock(&mm->mmap_sem).
Christophe
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v1 4/5] powerpc/fault: Avoid heavy search_exception_tables() verification
2020-09-09 6:20 ` Christophe Leroy
@ 2020-09-14 2:23 ` Nicholas Piggin
0 siblings, 0 replies; 29+ messages in thread
From: Nicholas Piggin @ 2020-09-14 2:23 UTC (permalink / raw)
To: Aneesh Kumar K.V, Benjamin Herrenschmidt, Christophe Leroy,
Michael Ellerman, Paul Mackerras
Cc: linux-kernel, linuxppc-dev
Excerpts from Christophe Leroy's message of September 9, 2020 4:20 pm:
>
>
> Le 09/09/2020 à 08:04, Aneesh Kumar K.V a écrit :
>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>
>>> search_exception_tables() is an heavy operation, we have to avoid it.
>>> When KUAP is selected, we'll know the fault has been blocked by KUAP.
>>> Otherwise, it behaves just as if the address was already in the TLBs
>>> and no fault was generated.
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>> ---
>>> arch/powerpc/mm/fault.c | 20 +++++---------------
>>> 1 file changed, 5 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>>> index 525e0c2b5406..edde169ba3a6 100644
>>> --- a/arch/powerpc/mm/fault.c
>>> +++ b/arch/powerpc/mm/fault.c
>>> @@ -214,24 +214,14 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
>>> if (address >= TASK_SIZE)
>>> return true;
>>>
>>> - if (!is_exec && (error_code & DSISR_PROTFAULT) &&
>>> - !search_exception_tables(regs->nip)) {
>>> + // Read/write fault blocked by KUAP is bad, it can never succeed.
>>> + if (bad_kuap_fault(regs, address, is_write)) {
>>> pr_crit_ratelimited("Kernel attempted to access user page (%lx) - exploit attempt? (uid: %d)\n",
>>> - address,
>>> - from_kuid(&init_user_ns, current_uid()));
>>> - }
>>> -
>>> - // Fault on user outside of certain regions (eg. copy_tofrom_user()) is bad
>>> - if (!search_exception_tables(regs->nip))
>>> - return true;
>>
>> We still need to keep this ? Without that we detect the lack of
>> exception tables pretty late.
>
> Is that a problem at all to detect the lack of exception tables late ?
> That case is very unlikely and will lead to failure anyway. So, is it
> worth impacting performance of the likely case which will always have an
> exception table and where we expect the exception to run as fast as
> possible ?
>
> The other architectures I have looked at (arm64 and x86) only have the
> exception table search together with the down_read_trylock(&mm->mmap_sem).
Yeah I don't see how it'd be a problem. User could arrange for page
table to already be at this address and avoid the fault so it's not the
right way to stop an attacker, KUAP is.
Thanks,
Nick
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v1 4/5] powerpc/fault: Avoid heavy search_exception_tables() verification
@ 2020-09-14 2:23 ` Nicholas Piggin
0 siblings, 0 replies; 29+ messages in thread
From: Nicholas Piggin @ 2020-09-14 2:23 UTC (permalink / raw)
To: Aneesh Kumar K.V, Benjamin Herrenschmidt, Christophe Leroy,
Michael Ellerman, Paul Mackerras
Cc: linuxppc-dev, linux-kernel
Excerpts from Christophe Leroy's message of September 9, 2020 4:20 pm:
>
>
> Le 09/09/2020 à 08:04, Aneesh Kumar K.V a écrit :
>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>
>>> search_exception_tables() is an heavy operation, we have to avoid it.
>>> When KUAP is selected, we'll know the fault has been blocked by KUAP.
>>> Otherwise, it behaves just as if the address was already in the TLBs
>>> and no fault was generated.
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>> ---
>>> arch/powerpc/mm/fault.c | 20 +++++---------------
>>> 1 file changed, 5 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>>> index 525e0c2b5406..edde169ba3a6 100644
>>> --- a/arch/powerpc/mm/fault.c
>>> +++ b/arch/powerpc/mm/fault.c
>>> @@ -214,24 +214,14 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
>>> if (address >= TASK_SIZE)
>>> return true;
>>>
>>> - if (!is_exec && (error_code & DSISR_PROTFAULT) &&
>>> - !search_exception_tables(regs->nip)) {
>>> + // Read/write fault blocked by KUAP is bad, it can never succeed.
>>> + if (bad_kuap_fault(regs, address, is_write)) {
>>> pr_crit_ratelimited("Kernel attempted to access user page (%lx) - exploit attempt? (uid: %d)\n",
>>> - address,
>>> - from_kuid(&init_user_ns, current_uid()));
>>> - }
>>> -
>>> - // Fault on user outside of certain regions (eg. copy_tofrom_user()) is bad
>>> - if (!search_exception_tables(regs->nip))
>>> - return true;
>>
>> We still need to keep this ? Without that we detect the lack of
>> exception tables pretty late.
>
> Is that a problem at all to detect the lack of exception tables late ?
> That case is very unlikely and will lead to failure anyway. So, is it
> worth impacting performance of the likely case which will always have an
> exception table and where we expect the exception to run as fast as
> possible ?
>
> The other architectures I have looked at (arm64 and x86) only have the
> exception table search together with the down_read_trylock(&mm->mmap_sem).
Yeah I don't see how it'd be a problem. User could arrange for page
table to already be at this address and avoid the fault so it's not the
right way to stop an attacker, KUAP is.
Thanks,
Nick
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2020-09-14 2:25 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-06 17:15 [PATCH v1 1/5] powerpc/mm: sanity_check_fault() should work for all, not only BOOK3S Christophe Leroy
2020-08-06 17:15 ` Christophe Leroy
2020-08-06 17:15 ` [PATCH v1 2/5] powerpc/fault: Unnest definition of page_fault_is_write() and page_fault_is_bad() Christophe Leroy
2020-08-06 17:15 ` Christophe Leroy
2020-09-08 8:44 ` Nicholas Piggin
2020-09-08 8:44 ` Nicholas Piggin
2020-08-06 17:15 ` [PATCH v1 3/5] powerpc/fault: Reorder tests in bad_kernel_fault() Christophe Leroy
2020-08-06 17:15 ` Christophe Leroy
2020-09-08 8:46 ` Nicholas Piggin
2020-09-08 8:46 ` Nicholas Piggin
2020-08-06 17:15 ` [PATCH v1 4/5] powerpc/fault: Avoid heavy search_exception_tables() verification Christophe Leroy
2020-08-06 17:15 ` Christophe Leroy
2020-09-08 8:54 ` Nicholas Piggin
2020-09-08 8:54 ` Nicholas Piggin
2020-09-09 6:04 ` Aneesh Kumar K.V
2020-09-09 6:20 ` Christophe Leroy
2020-09-14 2:23 ` Nicholas Piggin
2020-09-14 2:23 ` Nicholas Piggin
2020-08-06 17:15 ` [PATCH v1 5/5] powerpc/fault: Perform exception fixup in do_page_fault() Christophe Leroy
2020-08-06 17:15 ` Christophe Leroy
2020-08-06 21:07 ` kernel test robot
2020-08-06 21:07 ` kernel test robot
2020-08-06 21:07 ` kernel test robot
2020-09-08 9:13 ` Nicholas Piggin
2020-09-08 9:13 ` Nicholas Piggin
2020-09-08 8:43 ` [PATCH v1 1/5] powerpc/mm: sanity_check_fault() should work for all, not only BOOK3S Nicholas Piggin
2020-09-08 8:43 ` Nicholas Piggin
2020-09-08 8:56 ` Christophe Leroy
2020-09-08 8:56 ` Christophe Leroy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.