* [PATCH 1/2] powerpc: Rename thread_struct.fs to addr_limit
@ 2018-05-14 13:03 Michael Ellerman
2018-05-14 13:03 ` [PATCH 2/2] powerpc: Check address limit on user-mode return (TIF_FSCHECK) Michael Ellerman
2018-06-04 14:10 ` [1/2] powerpc: Rename thread_struct.fs to addr_limit Michael Ellerman
0 siblings, 2 replies; 4+ messages in thread
From: Michael Ellerman @ 2018-05-14 13:03 UTC (permalink / raw)
To: linuxppc-dev; +Cc: kernel-hardening, thgarnie, keescook
It's called 'fs' for historical reasons, it's named after the x86 'FS'
register. But we don't have to use that name for the member of
thread_struct, and in fact arch/x86 doesn't even call it 'fs' anymore.
So rename it to 'addr_limit', which better reflects what it's used
for, and is also the name used on other arches.
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/include/asm/processor.h | 6 +++---
arch/powerpc/include/asm/uaccess.h | 4 ++--
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index b4778cfaad5b..07167c2d1825 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -264,7 +264,7 @@ struct thread_struct {
unsigned long ksp_vsid;
#endif
struct pt_regs *regs; /* Pointer to saved register state */
- mm_segment_t fs; /* for get_fs() validation */
+ mm_segment_t addr_limit; /* for get_fs() validation */
#ifdef CONFIG_BOOKE
/* BookE base exception scratch space; align on cacheline */
unsigned long normsave[8] ____cacheline_aligned;
@@ -398,7 +398,7 @@ struct thread_struct {
#define INIT_THREAD { \
.ksp = INIT_SP, \
.ksp_limit = INIT_SP_LIMIT, \
- .fs = KERNEL_DS, \
+ .addr_limit = KERNEL_DS, \
.pgdir = swapper_pg_dir, \
.fpexc_mode = MSR_FE0 | MSR_FE1, \
SPEFSCR_INIT \
@@ -407,7 +407,7 @@ struct thread_struct {
#define INIT_THREAD { \
.ksp = INIT_SP, \
.regs = (struct pt_regs *)INIT_SP - 1, /* XXX bogus, I think */ \
- .fs = KERNEL_DS, \
+ .addr_limit = KERNEL_DS, \
.fpexc_mode = 0, \
.ppr = INIT_PPR, \
.fscr = FSCR_TAR | FSCR_EBB \
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index a62ee663b2c8..a91cea15187b 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -30,8 +30,8 @@
#endif
#define get_ds() (KERNEL_DS)
-#define get_fs() (current->thread.fs)
-#define set_fs(val) (current->thread.fs = (val))
+#define get_fs() (current->thread.addr_limit)
+#define set_fs(val) (current->thread.addr_limit = (val))
#define segment_eq(a, b) ((a).seg == (b).seg)
--
2.14.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] powerpc: Check address limit on user-mode return (TIF_FSCHECK)
2018-05-14 13:03 [PATCH 1/2] powerpc: Rename thread_struct.fs to addr_limit Michael Ellerman
@ 2018-05-14 13:03 ` Michael Ellerman
2018-05-15 22:04 ` Thomas Garnier
2018-06-04 14:10 ` [1/2] powerpc: Rename thread_struct.fs to addr_limit Michael Ellerman
1 sibling, 1 reply; 4+ messages in thread
From: Michael Ellerman @ 2018-05-14 13:03 UTC (permalink / raw)
To: linuxppc-dev; +Cc: kernel-hardening, thgarnie, keescook
set_fs() sets the addr_limit, which is used in access_ok() to
determine if an address is a user or kernel address.
Some code paths use set_fs() to temporarily elevate the addr_limit so
that kernel code can read/write kernel memory as if it were user
memory. That is fine as long as the code can't ever return to
userspace with the addr_limit still elevated.
If that did happen, then userspace can read/write kernel memory as if
it were user memory, eg. just with write(2). In case it's not clear,
that is very bad. It has also happened in the past due to bugs.
Commit 5ea0727b163c ("x86/syscalls: Check address limit on user-mode
return") added a mechanism to check the addr_limit value before
returning to userspace. Any call to set_fs() sets a thread flag,
TIF_FSCHECK, and if we see that on the return to userspace we go out
of line to check that the addr_limit value is not elevated.
For further info see the above commit, as well as:
https://lwn.net/Articles/722267/
https://bugs.chromium.org/p/project-zero/issues/detail?id=990
Verified to work on 64-bit Book3S using a POC that objdumps the system
call handler, and a modified lkdtm_CORRUPT_USER_DS() that doesn't kill
the caller.
Before:
$ sudo ./test-tif-fscheck
...
0000000000000000 <.data>:
0: e1 f7 8a 79 rldicl. r10,r12,30,63
4: 80 03 82 40 bne 0x384
8: 00 40 8a 71 andi. r10,r12,16384
c: 78 0b 2a 7c mr r10,r1
10: 10 fd 21 38 addi r1,r1,-752
14: 08 00 c2 41 beq- 0x1c
18: 58 09 2d e8 ld r1,2392(r13)
1c: 00 00 41 f9 std r10,0(r1)
20: 70 01 61 f9 std r11,368(r1)
24: 78 01 81 f9 std r12,376(r1)
28: 70 00 01 f8 std r0,112(r1)
2c: 78 00 41 f9 std r10,120(r1)
30: 20 00 82 41 beq 0x50
34: a6 42 4c 7d mftb r10
After:
$ sudo ./test-tif-fscheck
Killed
And in dmesg:
Invalid address limit on user-mode return
WARNING: CPU: 1 PID: 3689 at ../include/linux/syscalls.h:260 do_notify_resume+0x140/0x170
...
NIP [c00000000001ee50] do_notify_resume+0x140/0x170
LR [c00000000001ee4c] do_notify_resume+0x13c/0x170
Call Trace:
do_notify_resume+0x13c/0x170 (unreliable)
ret_from_except_lite+0x70/0x74
Performance overhead is essentially zero in the usual case, because
the bit is checked as part of the existing _TIF_USER_WORK_MASK check.
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/include/asm/thread_info.h | 8 +++++---
arch/powerpc/include/asm/uaccess.h | 8 +++++++-
arch/powerpc/kernel/signal.c | 4 ++++
3 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/thread_info.h b/arch/powerpc/include/asm/thread_info.h
index 5964145db03d..f308dfeb2746 100644
--- a/arch/powerpc/include/asm/thread_info.h
+++ b/arch/powerpc/include/asm/thread_info.h
@@ -79,8 +79,7 @@ extern int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src
#define TIF_SYSCALL_TRACE 0 /* syscall trace active */
#define TIF_SIGPENDING 1 /* signal pending */
#define TIF_NEED_RESCHED 2 /* rescheduling necessary */
-#define TIF_POLLING_NRFLAG 3 /* true if poll_idle() is polling
- TIF_NEED_RESCHED */
+#define TIF_FSCHECK 3 /* Check FS is USER_DS on return */
#define TIF_32BIT 4 /* 32 bit binary */
#define TIF_RESTORE_TM 5 /* need to restore TM FP/VEC/VSX */
#define TIF_PATCH_PENDING 6 /* pending live patching update */
@@ -99,6 +98,7 @@ extern int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src
#if defined(CONFIG_PPC64)
#define TIF_ELF2ABI 18 /* function descriptors must die! */
#endif
+#define TIF_POLLING_NRFLAG 19 /* true if poll_idle() is polling TIF_NEED_RESCHED */
/* as above, but as bit values */
#define _TIF_SYSCALL_TRACE (1<<TIF_SYSCALL_TRACE)
@@ -118,13 +118,15 @@ extern int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src
#define _TIF_SYSCALL_TRACEPOINT (1<<TIF_SYSCALL_TRACEPOINT)
#define _TIF_EMULATE_STACK_STORE (1<<TIF_EMULATE_STACK_STORE)
#define _TIF_NOHZ (1<<TIF_NOHZ)
+#define _TIF_FSCHECK (1<<TIF_FSCHECK)
#define _TIF_SYSCALL_DOTRACE (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \
_TIF_SECCOMP | _TIF_SYSCALL_TRACEPOINT | \
_TIF_NOHZ)
#define _TIF_USER_WORK_MASK (_TIF_SIGPENDING | _TIF_NEED_RESCHED | \
_TIF_NOTIFY_RESUME | _TIF_UPROBE | \
- _TIF_RESTORE_TM | _TIF_PATCH_PENDING)
+ _TIF_RESTORE_TM | _TIF_PATCH_PENDING | \
+ _TIF_FSCHECK)
#define _TIF_PERSYSCALL_MASK (_TIF_RESTOREALL|_TIF_NOERROR)
/* Bits in local_flags */
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index a91cea15187b..abba80f8ff04 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -31,7 +31,13 @@
#define get_ds() (KERNEL_DS)
#define get_fs() (current->thread.addr_limit)
-#define set_fs(val) (current->thread.addr_limit = (val))
+
+static inline void set_fs(mm_segment_t fs)
+{
+ current->thread.addr_limit = fs;
+ /* On user-mode return check addr_limit (fs) is correct */
+ set_thread_flag(TIF_FSCHECK);
+}
#define segment_eq(a, b) ((a).seg == (b).seg)
diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
index 61db86ecd318..fb932f1202c7 100644
--- a/arch/powerpc/kernel/signal.c
+++ b/arch/powerpc/kernel/signal.c
@@ -15,6 +15,7 @@
#include <linux/key.h>
#include <linux/context_tracking.h>
#include <linux/livepatch.h>
+#include <linux/syscalls.h>
#include <asm/hw_breakpoint.h>
#include <linux/uaccess.h>
#include <asm/unistd.h>
@@ -150,6 +151,9 @@ void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
{
user_exit();
+ /* Check valid addr_limit, TIF check is done there */
+ addr_limit_user_check();
+
if (thread_info_flags & _TIF_UPROBE)
uprobe_notify_resume(regs);
--
2.14.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] powerpc: Check address limit on user-mode return (TIF_FSCHECK)
2018-05-14 13:03 ` [PATCH 2/2] powerpc: Check address limit on user-mode return (TIF_FSCHECK) Michael Ellerman
@ 2018-05-15 22:04 ` Thomas Garnier
0 siblings, 0 replies; 4+ messages in thread
From: Thomas Garnier @ 2018-05-15 22:04 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, Kernel Hardening, Kees Cook
On Mon, May 14, 2018 at 6:03 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
> set_fs() sets the addr_limit, which is used in access_ok() to
> determine if an address is a user or kernel address.
> Some code paths use set_fs() to temporarily elevate the addr_limit so
> that kernel code can read/write kernel memory as if it were user
> memory. That is fine as long as the code can't ever return to
> userspace with the addr_limit still elevated.
> If that did happen, then userspace can read/write kernel memory as if
> it were user memory, eg. just with write(2). In case it's not clear,
> that is very bad. It has also happened in the past due to bugs.
> Commit 5ea0727b163c ("x86/syscalls: Check address limit on user-mode
> return") added a mechanism to check the addr_limit value before
> returning to userspace. Any call to set_fs() sets a thread flag,
> TIF_FSCHECK, and if we see that on the return to userspace we go out
> of line to check that the addr_limit value is not elevated.
> For further info see the above commit, as well as:
> https://lwn.net/Articles/722267/
> https://bugs.chromium.org/p/project-zero/issues/detail?id=990
> Verified to work on 64-bit Book3S using a POC that objdumps the system
> call handler, and a modified lkdtm_CORRUPT_USER_DS() that doesn't kill
> the caller.
> Before:
> $ sudo ./test-tif-fscheck
> ...
> 0000000000000000 <.data>:
> 0: e1 f7 8a 79 rldicl. r10,r12,30,63
> 4: 80 03 82 40 bne 0x384
> 8: 00 40 8a 71 andi. r10,r12,16384
> c: 78 0b 2a 7c mr r10,r1
> 10: 10 fd 21 38 addi r1,r1,-752
> 14: 08 00 c2 41 beq- 0x1c
> 18: 58 09 2d e8 ld r1,2392(r13)
> 1c: 00 00 41 f9 std r10,0(r1)
> 20: 70 01 61 f9 std r11,368(r1)
> 24: 78 01 81 f9 std r12,376(r1)
> 28: 70 00 01 f8 std r0,112(r1)
> 2c: 78 00 41 f9 std r10,120(r1)
> 30: 20 00 82 41 beq 0x50
> 34: a6 42 4c 7d mftb r10
> After:
> $ sudo ./test-tif-fscheck
> Killed
> And in dmesg:
> Invalid address limit on user-mode return
> WARNING: CPU: 1 PID: 3689 at ../include/linux/syscalls.h:260
do_notify_resume+0x140/0x170
> ...
> NIP [c00000000001ee50] do_notify_resume+0x140/0x170
> LR [c00000000001ee4c] do_notify_resume+0x13c/0x170
> Call Trace:
> do_notify_resume+0x13c/0x170 (unreliable)
> ret_from_except_lite+0x70/0x74
> Performance overhead is essentially zero in the usual case, because
> the bit is checked as part of the existing _TIF_USER_WORK_MASK check.
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
The change looks good to me.
> ---
> arch/powerpc/include/asm/thread_info.h | 8 +++++---
> arch/powerpc/include/asm/uaccess.h | 8 +++++++-
> arch/powerpc/kernel/signal.c | 4 ++++
> 3 files changed, 16 insertions(+), 4 deletions(-)
> diff --git a/arch/powerpc/include/asm/thread_info.h
b/arch/powerpc/include/asm/thread_info.h
> index 5964145db03d..f308dfeb2746 100644
> --- a/arch/powerpc/include/asm/thread_info.h
> +++ b/arch/powerpc/include/asm/thread_info.h
> @@ -79,8 +79,7 @@ extern int arch_dup_task_struct(struct task_struct
*dst, struct task_struct *src
> #define TIF_SYSCALL_TRACE 0 /* syscall trace active */
> #define TIF_SIGPENDING 1 /* signal pending */
> #define TIF_NEED_RESCHED 2 /* rescheduling necessary */
> -#define TIF_POLLING_NRFLAG 3 /* true if poll_idle() is polling
> - TIF_NEED_RESCHED */
> +#define TIF_FSCHECK 3 /* Check FS is USER_DS on return
*/
> #define TIF_32BIT 4 /* 32 bit binary */
> #define TIF_RESTORE_TM 5 /* need to restore TM FP/VEC/VSX
*/
> #define TIF_PATCH_PENDING 6 /* pending live patching update */
> @@ -99,6 +98,7 @@ extern int arch_dup_task_struct(struct task_struct
*dst, struct task_struct *src
> #if defined(CONFIG_PPC64)
> #define TIF_ELF2ABI 18 /* function descriptors must die!
*/
> #endif
> +#define TIF_POLLING_NRFLAG 19 /* true if poll_idle() is polling
TIF_NEED_RESCHED */
> /* as above, but as bit values */
> #define _TIF_SYSCALL_TRACE (1<<TIF_SYSCALL_TRACE)
> @@ -118,13 +118,15 @@ extern int arch_dup_task_struct(struct task_struct
*dst, struct task_struct *src
> #define _TIF_SYSCALL_TRACEPOINT (1<<TIF_SYSCALL_TRACEPOINT)
> #define _TIF_EMULATE_STACK_STORE (1<<TIF_EMULATE_STACK_STORE)
> #define _TIF_NOHZ (1<<TIF_NOHZ)
> +#define _TIF_FSCHECK (1<<TIF_FSCHECK)
> #define _TIF_SYSCALL_DOTRACE (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT
| \
> _TIF_SECCOMP | _TIF_SYSCALL_TRACEPOINT |
\
> _TIF_NOHZ)
> #define _TIF_USER_WORK_MASK (_TIF_SIGPENDING | _TIF_NEED_RESCHED | \
> _TIF_NOTIFY_RESUME | _TIF_UPROBE | \
> - _TIF_RESTORE_TM | _TIF_PATCH_PENDING)
> + _TIF_RESTORE_TM | _TIF_PATCH_PENDING | \
> + _TIF_FSCHECK)
> #define _TIF_PERSYSCALL_MASK (_TIF_RESTOREALL|_TIF_NOERROR)
> /* Bits in local_flags */
> diff --git a/arch/powerpc/include/asm/uaccess.h
b/arch/powerpc/include/asm/uaccess.h
> index a91cea15187b..abba80f8ff04 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -31,7 +31,13 @@
> #define get_ds() (KERNEL_DS)
> #define get_fs() (current->thread.addr_limit)
> -#define set_fs(val) (current->thread.addr_limit = (val))
> +
> +static inline void set_fs(mm_segment_t fs)
> +{
> + current->thread.addr_limit = fs;
> + /* On user-mode return check addr_limit (fs) is correct */
> + set_thread_flag(TIF_FSCHECK);
> +}
> #define segment_eq(a, b) ((a).seg == (b).seg)
> diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
> index 61db86ecd318..fb932f1202c7 100644
> --- a/arch/powerpc/kernel/signal.c
> +++ b/arch/powerpc/kernel/signal.c
> @@ -15,6 +15,7 @@
> #include <linux/key.h>
> #include <linux/context_tracking.h>
> #include <linux/livepatch.h>
> +#include <linux/syscalls.h>
> #include <asm/hw_breakpoint.h>
> #include <linux/uaccess.h>
> #include <asm/unistd.h>
> @@ -150,6 +151,9 @@ void do_notify_resume(struct pt_regs *regs, unsigned
long thread_info_flags)
> {
> user_exit();
> + /* Check valid addr_limit, TIF check is done there */
> + addr_limit_user_check();
> +
> if (thread_info_flags & _TIF_UPROBE)
> uprobe_notify_resume(regs);
> --
> 2.14.1
--
Thomas
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [1/2] powerpc: Rename thread_struct.fs to addr_limit
2018-05-14 13:03 [PATCH 1/2] powerpc: Rename thread_struct.fs to addr_limit Michael Ellerman
2018-05-14 13:03 ` [PATCH 2/2] powerpc: Check address limit on user-mode return (TIF_FSCHECK) Michael Ellerman
@ 2018-06-04 14:10 ` Michael Ellerman
1 sibling, 0 replies; 4+ messages in thread
From: Michael Ellerman @ 2018-06-04 14:10 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: thgarnie, keescook, kernel-hardening
On Mon, 2018-05-14 at 13:03:15 UTC, Michael Ellerman wrote:
> It's called 'fs' for historical reasons, it's named after the x86 'FS'
> register. But we don't have to use that name for the member of
> thread_struct, and in fact arch/x86 doesn't even call it 'fs' anymore.
>
> So rename it to 'addr_limit', which better reflects what it's used
> for, and is also the name used on other arches.
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Series applied to powerpc next.
https://git.kernel.org/powerpc/c/ba0635fcbe8c1ce83523c1ec797538
cheers
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-06-04 14:11 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-14 13:03 [PATCH 1/2] powerpc: Rename thread_struct.fs to addr_limit Michael Ellerman
2018-05-14 13:03 ` [PATCH 2/2] powerpc: Check address limit on user-mode return (TIF_FSCHECK) Michael Ellerman
2018-05-15 22:04 ` Thomas Garnier
2018-06-04 14:10 ` [1/2] powerpc: Rename thread_struct.fs to addr_limit Michael Ellerman
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.