diff mbox

[v10,1/3] x86/syscalls: Check address limit on user-mode return

Message ID 20170615011203.144108-1-thgarnie@google.com
State New, archived
Headers show

Commit Message

Thomas Garnier June 15, 2017, 1:12 a.m. UTC
Ensure the address limit is a user-mode segment before returning to
user-mode. Otherwise a process can corrupt kernel-mode memory and elevate
privileges [1].

The set_fs function sets the TIF_SETFS flag to force a slow path on
return. In the slow path, the address limit is checked to be USER_DS if
needed.

The addr_limit_user_check function is added as a cross-architecture
function to check the address limit.

[1] https://bugs.chromium.org/p/project-zero/issues/detail?id=990

Signed-off-by: Thomas Garnier <thgarnie@google.com>
---
v10 redesigns the change to use work flags on set_fs as recommended by
Linus and agreed by others.

Based on next-20170609
---
 arch/x86/entry/common.c            |  3 +++
 arch/x86/include/asm/thread_info.h |  5 ++++-
 arch/x86/include/asm/uaccess.h     |  7 ++++++-
 include/linux/syscalls.h           | 16 ++++++++++++++++
 4 files changed, 29 insertions(+), 2 deletions(-)

Comments

Kees Cook June 20, 2017, 8:24 p.m. UTC | #1
On Wed, Jun 14, 2017 at 6:12 PM, Thomas Garnier <thgarnie@google.com> wrote:
> Ensure the address limit is a user-mode segment before returning to
> user-mode. Otherwise a process can corrupt kernel-mode memory and elevate
> privileges [1].
>
> The set_fs function sets the TIF_SETFS flag to force a slow path on
> return. In the slow path, the address limit is checked to be USER_DS if
> needed.
>
> The addr_limit_user_check function is added as a cross-architecture
> function to check the address limit.
>
> [1] https://bugs.chromium.org/p/project-zero/issues/detail?id=990
>
> Signed-off-by: Thomas Garnier <thgarnie@google.com>

Thanks for reworking this series!

The bad state correctly BUGs under the LKDTM test:

[   21.171586] lkdtm: Performing direct entry CORRUPT_USER_DS
[   21.172791] lkdtm: setting bad task size limit
[   21.173742] ------------[ cut here ]------------
[   21.174641] kernel BUG at ./include/linux/syscalls.h:220!
...
[   21.193166] Call Trace:
[   21.193617]  ? trace_hardirqs_on_thunk+0x1a/0x1c
[   21.194443]  entry_SYSCALL64_slow_path+0x25/0x25


Tested-by: Kees Cook <keescook@chromium.org>

-Kees
Kees Cook June 28, 2017, 5:52 p.m. UTC | #2
On Tue, Jun 20, 2017 at 1:24 PM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Jun 14, 2017 at 6:12 PM, Thomas Garnier <thgarnie@google.com> wrote:
>> Ensure the address limit is a user-mode segment before returning to
>> user-mode. Otherwise a process can corrupt kernel-mode memory and elevate
>> privileges [1].
>>
>> The set_fs function sets the TIF_SETFS flag to force a slow path on
>> return. In the slow path, the address limit is checked to be USER_DS if
>> needed.
>>
>> The addr_limit_user_check function is added as a cross-architecture
>> function to check the address limit.
>>
>> [1] https://bugs.chromium.org/p/project-zero/issues/detail?id=990
>>
>> Signed-off-by: Thomas Garnier <thgarnie@google.com>
>
> Thanks for reworking this series!
>
> The bad state correctly BUGs under the LKDTM test:
>
> [   21.171586] lkdtm: Performing direct entry CORRUPT_USER_DS
> [   21.172791] lkdtm: setting bad task size limit
> [   21.173742] ------------[ cut here ]------------
> [   21.174641] kernel BUG at ./include/linux/syscalls.h:220!
> ...
> [   21.193166] Call Trace:
> [   21.193617]  ? trace_hardirqs_on_thunk+0x1a/0x1c
> [   21.194443]  entry_SYSCALL64_slow_path+0x25/0x25
>
>
> Tested-by: Kees Cook <keescook@chromium.org>

Is everyone happy with this patch for x86? Does this need anything
more/different?

Thanks!

-Kees
Thomas Garnier July 6, 2017, 8:38 p.m. UTC | #3
On Wed, Jun 28, 2017 at 10:52 AM, Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, Jun 20, 2017 at 1:24 PM, Kees Cook <keescook@chromium.org> wrote:
> > On Wed, Jun 14, 2017 at 6:12 PM, Thomas Garnier <thgarnie@google.com> wrote:
> >> Ensure the address limit is a user-mode segment before returning to
> >> user-mode. Otherwise a process can corrupt kernel-mode memory and elevate
> >> privileges [1].
> >>
> >> The set_fs function sets the TIF_SETFS flag to force a slow path on
> >> return. In the slow path, the address limit is checked to be USER_DS if
> >> needed.
> >>
> >> The addr_limit_user_check function is added as a cross-architecture
> >> function to check the address limit.
> >>
> >> [1] https://bugs.chromium.org/p/project-zero/issues/detail?id=990
> >>
> >> Signed-off-by: Thomas Garnier <thgarnie@google.com>
> >
> > Thanks for reworking this series!
> >
> > The bad state correctly BUGs under the LKDTM test:
> >
> > [   21.171586] lkdtm: Performing direct entry CORRUPT_USER_DS
> > [   21.172791] lkdtm: setting bad task size limit
> > [   21.173742] ------------[ cut here ]------------
> > [   21.174641] kernel BUG at ./include/linux/syscalls.h:220!
> > ...
> > [   21.193166] Call Trace:
> > [   21.193617]  ? trace_hardirqs_on_thunk+0x1a/0x1c
> > [   21.194443]  entry_SYSCALL64_slow_path+0x25/0x25
> >
> >
> > Tested-by: Kees Cook <keescook@chromium.org>
>
> Is everyone happy with this patch for x86? Does this need anything
> more/different?

Asking again. Additional feedback? Anyone wants to pick-it up?

>
> Thanks!
>
> -Kees
>
> --
> Kees Cook
> Pixel Security
Thomas Gleixner July 6, 2017, 8:48 p.m. UTC | #4
On Thu, 6 Jul 2017, Thomas Garnier wrote:
> On Wed, Jun 28, 2017 at 10:52 AM, Kees Cook <keescook@chromium.org> wrote:
> >
> > On Tue, Jun 20, 2017 at 1:24 PM, Kees Cook <keescook@chromium.org> wrote:
> > > On Wed, Jun 14, 2017 at 6:12 PM, Thomas Garnier <thgarnie@google.com> wrote:
> > >> Ensure the address limit is a user-mode segment before returning to
> > >> user-mode. Otherwise a process can corrupt kernel-mode memory and elevate
> > >> privileges [1].
> > >>
> > >> The set_fs function sets the TIF_SETFS flag to force a slow path on
> > >> return. In the slow path, the address limit is checked to be USER_DS if
> > >> needed.
> > >>
> > >> The addr_limit_user_check function is added as a cross-architecture
> > >> function to check the address limit.
> > >>
> > >> [1] https://bugs.chromium.org/p/project-zero/issues/detail?id=990
> > >>
> > >> Signed-off-by: Thomas Garnier <thgarnie@google.com>
> > >
> > > Thanks for reworking this series!
> > >
> > > The bad state correctly BUGs under the LKDTM test:
> > >
> > > [   21.171586] lkdtm: Performing direct entry CORRUPT_USER_DS
> > > [   21.172791] lkdtm: setting bad task size limit
> > > [   21.173742] ------------[ cut here ]------------
> > > [   21.174641] kernel BUG at ./include/linux/syscalls.h:220!
> > > ...
> > > [   21.193166] Call Trace:
> > > [   21.193617]  ? trace_hardirqs_on_thunk+0x1a/0x1c
> > > [   21.194443]  entry_SYSCALL64_slow_path+0x25/0x25
> > >
> > >
> > > Tested-by: Kees Cook <keescook@chromium.org>
> >
> > Is everyone happy with this patch for x86? Does this need anything
> > more/different?
> 
> Asking again. Additional feedback? Anyone wants to pick-it up?

Can do. This needs to be a combo of all 3 I assume as the x86 one contains
the function used by all of them, right?

Thanks,

	tglx
Thomas Garnier July 6, 2017, 8:52 p.m. UTC | #5
On Thu, Jul 6, 2017 at 1:48 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 6 Jul 2017, Thomas Garnier wrote:
>> On Wed, Jun 28, 2017 at 10:52 AM, Kees Cook <keescook@chromium.org> wrote:
>> >
>> > On Tue, Jun 20, 2017 at 1:24 PM, Kees Cook <keescook@chromium.org> wrote:
>> > > On Wed, Jun 14, 2017 at 6:12 PM, Thomas Garnier <thgarnie@google.com> wrote:
>> > >> Ensure the address limit is a user-mode segment before returning to
>> > >> user-mode. Otherwise a process can corrupt kernel-mode memory and elevate
>> > >> privileges [1].
>> > >>
>> > >> The set_fs function sets the TIF_SETFS flag to force a slow path on
>> > >> return. In the slow path, the address limit is checked to be USER_DS if
>> > >> needed.
>> > >>
>> > >> The addr_limit_user_check function is added as a cross-architecture
>> > >> function to check the address limit.
>> > >>
>> > >> [1] https://bugs.chromium.org/p/project-zero/issues/detail?id=990
>> > >>
>> > >> Signed-off-by: Thomas Garnier <thgarnie@google.com>
>> > >
>> > > Thanks for reworking this series!
>> > >
>> > > The bad state correctly BUGs under the LKDTM test:
>> > >
>> > > [   21.171586] lkdtm: Performing direct entry CORRUPT_USER_DS
>> > > [   21.172791] lkdtm: setting bad task size limit
>> > > [   21.173742] ------------[ cut here ]------------
>> > > [   21.174641] kernel BUG at ./include/linux/syscalls.h:220!
>> > > ...
>> > > [   21.193166] Call Trace:
>> > > [   21.193617]  ? trace_hardirqs_on_thunk+0x1a/0x1c
>> > > [   21.194443]  entry_SYSCALL64_slow_path+0x25/0x25
>> > >
>> > >
>> > > Tested-by: Kees Cook <keescook@chromium.org>
>> >
>> > Is everyone happy with this patch for x86? Does this need anything
>> > more/different?
>>
>> Asking again. Additional feedback? Anyone wants to pick-it up?
>
> Can do. This needs to be a combo of all 3 I assume as the x86 one contains
> the function used by all of them, right?

That is correct.

>
> Thanks,
>
>         tglx
diff mbox

Patch

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index cdefcfdd9e63..03505ffbe1b6 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -23,6 +23,7 @@ 
 #include <linux/user-return-notifier.h>
 #include <linux/uprobes.h>
 #include <linux/livepatch.h>
+#include <linux/syscalls.h>
 
 #include <asm/desc.h>
 #include <asm/traps.h>
@@ -183,6 +184,8 @@  __visible inline void prepare_exit_to_usermode(struct pt_regs *regs)
 	struct thread_info *ti = current_thread_info();
 	u32 cached_flags;
 
+	addr_limit_user_check();
+
 	if (IS_ENABLED(CONFIG_PROVE_LOCKING) && WARN_ON(!irqs_disabled()))
 		local_irq_disable();
 
diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index e00e1bd6e7b3..5161da1a0fa0 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -98,6 +98,7 @@  struct thread_info {
 #define TIF_SYSCALL_TRACEPOINT	28	/* syscall tracepoint instrumentation */
 #define TIF_ADDR32		29	/* 32-bit address space on 64 bits */
 #define TIF_X32			30	/* 32-bit native x86-64 binary */
+#define TIF_FSCHECK		31	/* Check FS is USER_DS on return */
 
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)
@@ -122,6 +123,7 @@  struct thread_info {
 #define _TIF_SYSCALL_TRACEPOINT	(1 << TIF_SYSCALL_TRACEPOINT)
 #define _TIF_ADDR32		(1 << TIF_ADDR32)
 #define _TIF_X32		(1 << TIF_X32)
+#define _TIF_FSCHECK		(1 << TIF_FSCHECK)
 
 /*
  * work to do in syscall_trace_enter().  Also includes TIF_NOHZ for
@@ -137,7 +139,8 @@  struct thread_info {
 	(_TIF_SYSCALL_TRACE | _TIF_NOTIFY_RESUME | _TIF_SIGPENDING |	\
 	 _TIF_NEED_RESCHED | _TIF_SINGLESTEP | _TIF_SYSCALL_EMU |	\
 	 _TIF_SYSCALL_AUDIT | _TIF_USER_RETURN_NOTIFY | _TIF_UPROBE |	\
-	 _TIF_PATCH_PENDING | _TIF_NOHZ | _TIF_SYSCALL_TRACEPOINT)
+	 _TIF_PATCH_PENDING | _TIF_NOHZ | _TIF_SYSCALL_TRACEPOINT |	\
+	 _TIF_FSCHECK)
 
 /* flags to check in __switch_to() */
 #define _TIF_WORK_CTXSW							\
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index a059aac9e937..11433f9018e2 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -26,7 +26,12 @@ 
 
 #define get_ds()	(KERNEL_DS)
 #define get_fs()	(current->thread.addr_limit)
-#define set_fs(x)	(current->thread.addr_limit = (x))
+static inline void set_fs(mm_segment_t fs)
+{
+	current->thread.addr_limit = fs;
+	/* On user-mode return, check fs is correct */
+	set_thread_flag(TIF_FSCHECK);
+}
 
 #define segment_eq(a, b)	((a).seg == (b).seg)
 
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 980c3c9b06f8..ac0cf6fb25d6 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -206,6 +206,22 @@  extern struct trace_event_functions exit_syscall_print_funcs;
 	}								\
 	static inline long SYSC##name(__MAP(x,__SC_DECL,__VA_ARGS__))
 
+#ifdef TIF_FSCHECK
+/*
+ * Called before coming back to user-mode. Returning to user-mode with an
+ * address limit different than USER_DS can allow to overwrite kernel memory.
+ */
+static inline void addr_limit_user_check(void)
+{
+
+	if (!test_thread_flag(TIF_FSCHECK))
+		return;
+
+	BUG_ON(!segment_eq(get_fs(), USER_DS));
+	clear_thread_flag(TIF_FSCHECK);
+}
+#endif
+
 asmlinkage long sys32_quotactl(unsigned int cmd, const char __user *special,
 			       qid_t id, void __user *addr);
 asmlinkage long sys_time(time_t __user *tloc);