[SRU][PATCH 0/1] Fix for CVE-2018-17972

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[SRU][PATCH 0/1] Fix for CVE-2018-17972

Khaled Elmously
Restrict /proc/*/stack to CAP_SYS_ADMIN

Jann Horn (1):
  proc: restrict kernel stack dumps to root

 fs/proc/base.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU][Trusty][Bionic][PATCH 1/1] proc: restrict kernel stack dumps to root

Khaled Elmously
From: Jann Horn <[hidden email]>

CVE-2018-17972

Currently, you can use /proc/self/task/*/stack to cause a stack walk on
a task you control while it is running on another CPU.  That means that
the stack can change under the stack walker.  The stack walker does
have guards against going completely off the rails and into random
kernel memory, but it can interpret random data from your kernel stack
as instruction pointers and stack pointers.  This can cause exposure of
kernel stack contents to userspace.

Restrict the ability to inspect kernel stacks of arbitrary tasks to root
in order to prevent a local attacker from exploiting racy stack unwinding
to leak kernel task stack contents.  See the added comment for a longer
rationale.

There don't seem to be any users of this userspace API that can't
gracefully bail out if reading from the file fails.  Therefore, I believe
that this change is unlikely to break things.  In the case that this patch
does end up needing a revert, the next-best solution might be to fake a
single-entry stack based on wchan.

Link: http://lkml.kernel.org/r/20180927153316.200286-1-jannh@...
Fixes: 2ec220e27f50 ("proc: add /proc/*/stack")
Signed-off-by: Jann Horn <[hidden email]>
Acked-by: Kees Cook <[hidden email]>
Cc: Alexey Dobriyan <[hidden email]>
Cc: Ken Chen <[hidden email]>
Cc: Will Deacon <[hidden email]>
Cc: Laura Abbott <[hidden email]>
Cc: Andy Lutomirski <[hidden email]>
Cc: Catalin Marinas <[hidden email]>
Cc: Josh Poimboeuf <[hidden email]>
Cc: Thomas Gleixner <[hidden email]>
Cc: Ingo Molnar <[hidden email]>
Cc: "H . Peter Anvin" <[hidden email]>
Cc: <[hidden email]>
Signed-off-by: Andrew Morton <[hidden email]>
Signed-off-by: Greg Kroah-Hartman <[hidden email]>
(backported from f8a00cef17206ecd1b30d3d9f99e10d9fa707aa7)
[ kmously: Minor context adjustment ]
Signed-off-by: Khalid Elmously <[hidden email]>
---
 fs/proc/base.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 4dd9d5541088..7c798e95a69e 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -304,6 +304,20 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
  int err;
  int i;
 
+ /*
+ * The ability to racily run the kernel stack unwinder on a running task
+ * and then observe the unwinder output is scary; while it is useful for
+ * debugging kernel issues, it can also allow an attacker to leak kernel
+ * stack contents.
+ * Doing this in a manner that is at least safe from races would require
+ * some work to ensure that the remote task can not be scheduled; and
+ * even then, this would still expose the unwinder as local attack
+ * surface.
+ * Therefore, this interface is restricted to root.
+ */
+ if (!file_ns_capable(m->file, &init_user_ns, CAP_SYS_ADMIN))
+ return -EACCES;
+
  entries = kmalloc(MAX_STACK_TRACE_DEPTH * sizeof(*entries), GFP_KERNEL);
  if (!entries)
  return -ENOMEM;
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [SRU][Trusty][Bionic][PATCH 1/1] proc: restrict kernel stack dumps to root

Juerg Haefliger
On Mon,  3 Dec 2018 21:21:04 -0500
Khalid Elmously <[hidden email]> wrote:

> From: Jann Horn <[hidden email]>
>
> CVE-2018-17972
>
> Currently, you can use /proc/self/task/*/stack to cause a stack walk on
> a task you control while it is running on another CPU.  That means that
> the stack can change under the stack walker.  The stack walker does
> have guards against going completely off the rails and into random
> kernel memory, but it can interpret random data from your kernel stack
> as instruction pointers and stack pointers.  This can cause exposure of
> kernel stack contents to userspace.
>
> Restrict the ability to inspect kernel stacks of arbitrary tasks to root
> in order to prevent a local attacker from exploiting racy stack unwinding
> to leak kernel task stack contents.  See the added comment for a longer
> rationale.
>
> There don't seem to be any users of this userspace API that can't
> gracefully bail out if reading from the file fails.  Therefore, I believe
> that this change is unlikely to break things.  In the case that this patch
> does end up needing a revert, the next-best solution might be to fake a
> single-entry stack based on wchan.
>
> Link: http://lkml.kernel.org/r/20180927153316.200286-1-jannh@...
> Fixes: 2ec220e27f50 ("proc: add /proc/*/stack")
> Signed-off-by: Jann Horn <[hidden email]>
> Acked-by: Kees Cook <[hidden email]>
> Cc: Alexey Dobriyan <[hidden email]>
> Cc: Ken Chen <[hidden email]>
> Cc: Will Deacon <[hidden email]>
> Cc: Laura Abbott <[hidden email]>
> Cc: Andy Lutomirski <[hidden email]>
> Cc: Catalin Marinas <[hidden email]>
> Cc: Josh Poimboeuf <[hidden email]>
> Cc: Thomas Gleixner <[hidden email]>
> Cc: Ingo Molnar <[hidden email]>
> Cc: "H . Peter Anvin" <[hidden email]>
> Cc: <[hidden email]>
> Signed-off-by: Andrew Morton <[hidden email]>
> Signed-off-by: Greg Kroah-Hartman <[hidden email]>
Empty line here please.

> (backported from f8a00cef17206ecd1b30d3d9f99e10d9fa707aa7)

(backported from commit ...

...Juerg


> [ kmously: Minor context adjustment ]
> Signed-off-by: Khalid Elmously <[hidden email]>
> ---
>  fs/proc/base.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 4dd9d5541088..7c798e95a69e 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -304,6 +304,20 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
>   int err;
>   int i;
>  
> + /*
> + * The ability to racily run the kernel stack unwinder on a running task
> + * and then observe the unwinder output is scary; while it is useful for
> + * debugging kernel issues, it can also allow an attacker to leak kernel
> + * stack contents.
> + * Doing this in a manner that is at least safe from races would require
> + * some work to ensure that the remote task can not be scheduled; and
> + * even then, this would still expose the unwinder as local attack
> + * surface.
> + * Therefore, this interface is restricted to root.
> + */
> + if (!file_ns_capable(m->file, &init_user_ns, CAP_SYS_ADMIN))
> + return -EACCES;
> +
>   entries = kmalloc(MAX_STACK_TRACE_DEPTH * sizeof(*entries), GFP_KERNEL);
>   if (!entries)
>   return -ENOMEM;

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

attachment0 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

ACK: [SRU][Trusty][Bionic][PATCH 1/1] proc: restrict kernel stack dumps to root

Po-Hsu Lin (Sam)
In reply to this post by Khaled Elmously
Backport looks ok with changes limited to the original patch.
Test build OK.

I think we don't have a convention to keep an empty line before (backport ...),
don't we?

https://wiki.ubuntu.com/Kernel/Dev/StablePatchFormat

Acked-by: Po-Hsu Lin <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [SRU][Trusty][Bionic][PATCH 1/1] proc: restrict kernel stack dumps to root

Kleber Souza
In reply to this post by Khaled Elmously
On 12/4/18 3:21 AM, Khalid Elmously wrote:

> From: Jann Horn <[hidden email]>
>
> CVE-2018-17972
>
> Currently, you can use /proc/self/task/*/stack to cause a stack walk on
> a task you control while it is running on another CPU.  That means that
> the stack can change under the stack walker.  The stack walker does
> have guards against going completely off the rails and into random
> kernel memory, but it can interpret random data from your kernel stack
> as instruction pointers and stack pointers.  This can cause exposure of
> kernel stack contents to userspace.
>
> Restrict the ability to inspect kernel stacks of arbitrary tasks to root
> in order to prevent a local attacker from exploiting racy stack unwinding
> to leak kernel task stack contents.  See the added comment for a longer
> rationale.
>
> There don't seem to be any users of this userspace API that can't
> gracefully bail out if reading from the file fails.  Therefore, I believe
> that this change is unlikely to break things.  In the case that this patch
> does end up needing a revert, the next-best solution might be to fake a
> single-entry stack based on wchan.
>
> Link: http://lkml.kernel.org/r/20180927153316.200286-1-jannh@...
> Fixes: 2ec220e27f50 ("proc: add /proc/*/stack")
> Signed-off-by: Jann Horn <[hidden email]>
> Acked-by: Kees Cook <[hidden email]>
> Cc: Alexey Dobriyan <[hidden email]>
> Cc: Ken Chen <[hidden email]>
> Cc: Will Deacon <[hidden email]>
> Cc: Laura Abbott <[hidden email]>
> Cc: Andy Lutomirski <[hidden email]>
> Cc: Catalin Marinas <[hidden email]>
> Cc: Josh Poimboeuf <[hidden email]>
> Cc: Thomas Gleixner <[hidden email]>
> Cc: Ingo Molnar <[hidden email]>
> Cc: "H . Peter Anvin" <[hidden email]>
> Cc: <[hidden email]>
> Signed-off-by: Andrew Morton <[hidden email]>
> Signed-off-by: Greg Kroah-Hartman <[hidden email]>
> (backported from f8a00cef17206ecd1b30d3d9f99e10d9fa707aa7)

As Juerg pointed out:

(backported from commit ...)

This can be fixed when applying the patch.

> [ kmously: Minor context adjustment ]
> Signed-off-by: Khalid Elmously <[hidden email]>

Acked-by: Kleber Sacilotto de Souza <[hidden email]>

> ---
>  fs/proc/base.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 4dd9d5541088..7c798e95a69e 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -304,6 +304,20 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
>   int err;
>   int i;
>  
> + /*
> + * The ability to racily run the kernel stack unwinder on a running task
> + * and then observe the unwinder output is scary; while it is useful for
> + * debugging kernel issues, it can also allow an attacker to leak kernel
> + * stack contents.
> + * Doing this in a manner that is at least safe from races would require
> + * some work to ensure that the remote task can not be scheduled; and
> + * even then, this would still expose the unwinder as local attack
> + * surface.
> + * Therefore, this interface is restricted to root.
> + */
> + if (!file_ns_capable(m->file, &init_user_ns, CAP_SYS_ADMIN))
> + return -EACCES;
> +
>   entries = kmalloc(MAX_STACK_TRACE_DEPTH * sizeof(*entries), GFP_KERNEL);
>   if (!entries)
>   return -ENOMEM;



--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED(T,B)/cmt: [SRU][PATCH 0/1] Fix for CVE-2018-17972

Khaled Elmously
In reply to this post by Khaled Elmously
Fixed 'backported from' line.


On 2018-12-03 21:21:03 , Khalid Elmously wrote:

> Restrict /proc/*/stack to CAP_SYS_ADMIN
>
> Jann Horn (1):
>   proc: restrict kernel stack dumps to root
>
>  fs/proc/base.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> --
> 2.17.1
>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team