powerpc/mm/hash: Hand user access of kernel address gracefully

PATCH Applied Aneesh Kumar K.V 4 participants 4 messages Nov 26, 2018 – Dec 22, 2018 PowerPC

Share & tools

Aneesh Kumar K.V

author

2018-11-26 14:37 UTC #0 lore raw

PATCH 1/1 powerpc/mm/hash: Hand user access of kernel address gracefully

With commit 2865d08dd9ea ("powerpc/mm: Move the DSISR_PROTFAULT sanity check") we moved the protection fault access check before vma lookup. That means we hit that WARN_ON when user space access a kernel address. Before the commit this was handled by find_vma() not finding vma for the kernel address and considering that access as bad area access.

Avoid the confusing WARN_ON and convert that to a ratelimited printk. With the patch we now get

for load:

[  187.700294] a.out[5997]: User access of kernel address (c00000000000dea0) - exploit attempt? (uid: 1000)
[  187.700344] a.out[5997]: segfault (11) at c00000000000dea0 nip 1317c0798 lr 7fff80d6441c code 1 in a.out[1317c0000+10000]
[  187.700429] a.out[5997]: code: 60000000 60420000 3c4c0002 38427790 4bffff20 3c4c0002 38427784 fbe1fff8
[  187.700435] a.out[5997]: code: f821ffc1 7c3f0b78 60000000 e9228030 <89290000> 993f002f 60000000 383f0040

for exec:

[  225.100903] a.out[6067]: User access of kernel address (c00000000000dea0) - exploit attempt? (uid: 1000)
[  225.100938] a.out[6067]: segfault (11) at c00000000000dea0 nip c00000000000dea0 lr 129d507b0 code 1
[  225.100943] a.out[6067]: Bad NIP, not dumping instructions.

Fixes 2865d08dd9ea ("powerpc/mm: Move the DSISR_PROTFAULT sanity check")

Signed-off-by Aneesh Kumar K.V <[email protected]>

---
 arch/powerpc/mm/fault.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

arch/powerpc/mm/fault.c +24 -5

index 1697e903bbf2..46f280068c45 100644
@@ -342,8 +342,21 @@ static inline void cmo_account_page_fault(void) { }
 #endif /* CONFIG_PPC_SMLPAR */
 
 #ifdef CONFIG_PPC_STD_MMU
-static void sanity_check_fault(bool is_write, unsigned long error_code)
+static void sanity_check_fault(bool is_write, bool is_user,
+			       unsigned long error_code, unsigned long address)
 {
+	/*
+	 * userspace trying to access kernel address, we get PROTFAULT for that.
+	 */
+	if (is_user && address >= TASK_SIZE) {
+		printk_ratelimited(KERN_CRIT "%s[%d]: "
+				   "User access of kernel address (%lx) - "
+				   "exploit attempt? (uid: %d)\n",
+				   current->comm, current->pid, address,
+				   from_kuid(&init_user_ns, current_uid()));
+		return;
+	}
+
 	/*
 	 * For hash translation mode, we should never get a
 	 * PROTFAULT. Any update to pte to reduce access will result in us

@@ -373,11 +386,17 @@ static void sanity_check_fault(bool is_write, unsigned long error_code)
 	 * For radix, we can get prot fault for autonuma case, because radix
 	 * page table will have them marked noaccess for user.
 	 */
-	if (!radix_enabled() && !is_write)
-		WARN_ON_ONCE(error_code & DSISR_PROTFAULT);
+	if (radix_enabled() || is_write)
+		return;
+
+	WARN_ON_ONCE(error_code & DSISR_PROTFAULT);
 }
 #else
-static void sanity_check_fault(bool is_write, unsigned long error_code) { }
+static void sanity_check_fault(bool is_write, bool is_user,
+			       unsigned long error_code, unsigned long address)
+{
+
+}
 #endif /* CONFIG_PPC_STD_MMU */
 
 /*

@@ -435,7 +454,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,
 	}
 
 	/* Additional sanity check(s) */
-	sanity_check_fault(is_write, error_code);
+	sanity_check_fault(is_write, is_user, error_code, address);
 
 	/*
 	 * The kernel should never take an execute fault nor should it

Signature · 1 line

2.19.1

Breno Leitao

Tested-by on v1 1/1

2018-11-27 17:24 UTC #1 lore raw

hi Aneesh,

Quoted 15 lines · Aneesh Kumar K.V

With commit 2865d08dd9ea ("powerpc/mm: Move the DSISR_PROTFAULT sanity check") we moved the protection fault access check before vma lookup. That means we hit that WARN_ON when user space access a kernel address. Before the commit this was handled by find_vma() not finding vma for the kernel address and considering that access as bad area access.
Avoid the confusing WARN_ON and convert that to a ratelimited printk. With the patch we now get
for load:
[  187.700294] a.out[5997]: User access of kernel address (c00000000000dea0) - exploit attempt? (uid: 1000)
[  187.700344] a.out[5997]: segfault (11) at c00000000000dea0 nip 1317c0798 lr 7fff80d6441c code 1 in a.out[1317c0000+10000]
[  187.700429] a.out[5997]: code: 60000000 60420000 3c4c0002 38427790 4bffff20 3c4c0002 38427784 fbe1fff8
[  187.700435] a.out[5997]: code: f821ffc1 7c3f0b78 60000000 e9228030 <89290000> 993f002f 60000000 383f0040

Quoted 7 lines

for exec:
[  225.100903] a.out[6067]: User access of kernel address (c00000000000dea0) - exploit attempt? (uid: 1000)
[  225.100938] a.out[6067]: segfault (11) at c00000000000dea0 nip c00000000000dea0 lr 129d507b0 code 1
[  225.100943] a.out[6067]: Bad NIP, not dumping instructions.
Fixes 2865d08dd9ea ("powerpc/mm: Move the DSISR_PROTFAULT sanity check")
Signed-off-by Aneesh Kumar K.V <[email protected]>

Tested-by Breno Leitao <[email protected]>

Quoted 16 lines

---
 arch/powerpc/mm/fault.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

arch/powerpc/mm/fault.c +2 -1

index 1697e903bbf2..46f280068c45 100644
@@ -342,8 +342,21 @@ static inline void cmo_account_page_fault(void) { }
 #endif /* CONFIG_PPC_SMLPAR */
 
 #ifdef CONFIG_PPC_STD_MMU
-static void sanity_check_fault(bool is_write, unsigned long error_code)
+static void sanity_check_fault(bool is_write, bool is_user,
+			       unsigned long error_code, unsigned long address)
 {

arch/powerpc/mm/fault.c static inline void cmo_account_page_fault(void) { } · line 357

Quoted 10 lines

arch/powerpc/mm/fault.c +10 -0

+	/*
+	 * userspace trying to access kernel address, we get PROTFAULT for that.
+	 */
+	if (is_user && address >= TASK_SIZE) {
+		printk_ratelimited(KERN_CRIT "%s[%d]: "
+				   "User access of kernel address (%lx) - "
+				   "exploit attempt? (uid: %d)\n",
+				   current->comm, current->pid, address,
+				   from_kuid(&init_user_ns, current_uid()));
+		return;

Silly question: Is it OK to printk() and just return here? __do_page_fault will continue to execute independently of this return, right?

Michael Ellerman

reviewer

2018-12-13 02:46 UTC #2 lore raw

Quoted 23 lines · Breno Leitao

hi Aneesh,
Quoted 20 lines · Aneesh Kumar K.V
With commit 2865d08dd9ea ("powerpc/mm: Move the DSISR_PROTFAULT sanity check") we moved the protection fault access check before vma lookup. That means we hit that WARN_ON when user space access a kernel address. Before the commit this was handled by find_vma() not finding vma for the kernel address and considering that access as bad area access.
Avoid the confusing WARN_ON and convert that to a ratelimited printk. With the patch we now get
for load:
[  187.700294] a.out[5997]: User access of kernel address (c00000000000dea0) - exploit attempt? (uid: 1000)
[  187.700344] a.out[5997]: segfault (11) at c00000000000dea0 nip 1317c0798 lr 7fff80d6441c code 1 in a.out[1317c0000+10000]
[  187.700429] a.out[5997]: code: 60000000 60420000 3c4c0002 38427790 4bffff20 3c4c0002 38427784 fbe1fff8
[  187.700435] a.out[5997]: code: f821ffc1 7c3f0b78 60000000 e9228030 <89290000> 993f002f 60000000 383f0040
for exec:
[  225.100903] a.out[6067]: User access of kernel address (c00000000000dea0) - exploit attempt? (uid: 1000)
[  225.100938] a.out[6067]: segfault (11) at c00000000000dea0 nip c00000000000dea0 lr 129d507b0 code 1
[  225.100943] a.out[6067]: Bad NIP, not dumping instructions.

Quoted 4 lines

Quoted 2 lines
Fixes 2865d08dd9ea ("powerpc/mm: Move the DSISR_PROTFAULT sanity check")
Signed-off-by Aneesh Kumar K.V <[email protected]>
Tested-by Breno Leitao <[email protected]>

Thanks.

Quoted 17 lines

arch/powerpc/mm/fault.c +7 -1

index 1697e903bbf2..46f280068c45 100644
@@ -342,8 +342,21 @@ static inline void cmo_account_page_fault(void) { }
 #endif /* CONFIG_PPC_SMLPAR */
 
 #ifdef CONFIG_PPC_STD_MMU
-static void sanity_check_fault(bool is_write, unsigned long error_code)
+static void sanity_check_fault(bool is_write, bool is_user,
+			       unsigned long error_code, unsigned long address)
 {
+	/*
+	 * userspace trying to access kernel address, we get PROTFAULT for that.
+	 */
+	if (is_user && address >= TASK_SIZE) {
+		printk_ratelimited(KERN_CRIT "%s[%d]: "

Quoted 8 lines

arch/powerpc/mm/fault.c static inline void cmo_account_page_fault(void) { } · line 357
Quoted 5 lines
arch/powerpc/mm/fault.c +5 -0
+				   "User access of kernel address (%lx) - "
+				   "exploit attempt? (uid: %d)\n",
+				   current->comm, current->pid, address,
+				   from_kuid(&init_user_ns, current_uid()));
+		return;
Silly question: Is it OK to printk() and just return here? __do_page_fault will continue to execute independently of this return, right?

Yeah it is OK to just return.

I agree it's a bit of a strange way for the code to be structured, ie. we detect a bad condition and print about it and then just return and let it continue anyway.

I guess it's that way because it was added as an additional check, ie. the code already handled those cases further down, but this was a check in case anything weird happened.

If you look at the start of __do_page_fault() we have three separate checks:

	if (unlikely(page_fault_is_bad(error_code))) {
		if (is_user) {
			_exception(SIGBUS, regs, BUS_OBJERR, address);
			return 0;
		}
		return SIGBUS;
	}

	/* Additional sanity check(s) */
	sanity_check_fault(is_write, is_user, error_code, address);

	/*
	 * The kernel should never take an execute fault nor should it
	 * take a page fault to a kernel address.
	 */
	if (unlikely(!is_user && bad_kernel_fault(is_exec, error_code, address)))
		return SIGSEGV;

It seems like maybe we could simplify that somewhat.

We need to be careful though that we return the right signal (SEGV or BUS), and also that user faults get counted (see PERF_COUNT_SW_PAGE_FAULTS).

So it's not straight forward as usual :)

cheers

Michael Ellerman

Applied

2018-12-22 09:57 UTC #3 lore raw

In reply to Aneesh Kumar K.V

Quoted 15 lines · Aneesh Kumar K.V

With commit 2865d08dd9ea ("powerpc/mm: Move the DSISR_PROTFAULT sanity check") we moved the protection fault access check before vma lookup. That means we hit that WARN_ON when user space access a kernel address. Before the commit this was handled by find_vma() not finding vma for the kernel address and considering that access as bad area access.
Avoid the confusing WARN_ON and convert that to a ratelimited printk. With the patch we now get
for load:
[  187.700294] a.out[5997]: User access of kernel address (c00000000000dea0) - exploit attempt? (uid: 1000)
[  187.700344] a.out[5997]: segfault (11) at c00000000000dea0 nip 1317c0798 lr 7fff80d6441c code 1 in a.out[1317c0000+10000]
[  187.700429] a.out[5997]: code: 60000000 60420000 3c4c0002 38427790 4bffff20 3c4c0002 38427784 fbe1fff8
[  187.700435] a.out[5997]: code: f821ffc1 7c3f0b78 60000000 e9228030 <89290000> 993f002f 60000000 383f0040

Quoted 8 lines

for exec:
[  225.100903] a.out[6067]: User access of kernel address (c00000000000dea0) - exploit attempt? (uid: 1000)
[  225.100938] a.out[6067]: segfault (11) at c00000000000dea0 nip c00000000000dea0 lr 129d507b0 code 1
[  225.100943] a.out[6067]: Bad NIP, not dumping instructions.
Fixes 2865d08dd9ea ("powerpc/mm: Move the DSISR_PROTFAULT sanity check")
Signed-off-by Aneesh Kumar K.V <[email protected]>
Tested-by Breno Leitao <[email protected]>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/374f3f5979f9b28bfb5b5799208d82

cheers