The original test was attempting to crash the kernel by setting a
breakpoint on do_debug kernel function which, when triggered, caused an
infinite loop in the kernel. The problem with this approach is that
kernel internal function names are not stable at all and the name was
changed recently, which made the test fail for no good reason.
The original kernel fix made it however poissible to set a kernel
address as a breakpoint and instead disabled the breakpoint on userspace
modification. The error checks were deffered to write to the dr7 that
enabled the breakpoint again.
So on newer kernels we do not allow to set the breakpoint to the kernel
addres at all, which means that the POKEUSR to dr0 has to fail with an
address in a kernel range and also we read back the breakpoint address
and check that it wasn't set just to be sure.
On older kernels we check that the POKEUSER to dr7 that enables the
breakpoint fails properly after the dr0 has been set to an address in
the kernel range.
Signed-off-by: Cyril Hrubis <[email protected]>
CC: Andy Lutomirski <[email protected]>
CC: Peter Zijlstra <[email protected]>
CC: Thomas Gleixner <[email protected]>
CC: Alexandre Chartre <[email protected]>
---
testcases/kernel/syscalls/ptrace/ptrace08.c | 136 +++++++++++---------
1 file changed, 76 insertions(+), 60 deletions(-)
diff --git a/testcases/kernel/syscalls/ptrace/ptrace08.c b/testcases/kernel/syscalls/ptrace/ptrace08.c
index 591aa0dd2..1b84ce376 100644
--- a/testcases/kernel/syscalls/ptrace/ptrace08.c
+++ b/testcases/kernel/syscalls/ptrace/ptrace08.c
@@ -5,8 +5,17 @@
*
* CVE-2018-1000199
*
- * Test error handling when ptrace(POKEUSER) modifies debug registers.
- * Even if the call returns error, it may create breakpoint in kernel code.
+ * Test error handling when ptrace(POKEUSER) modified x86 debug registers even
+ * when the call returned error.
+ *
+ * When the bug was present we could create breakpoint in the kernel code,
+ * which shoudn't be possible at all. The original CVE caused a kernel crash by
+ * setting a breakpoint on do_debug kernel function which, when triggered,
+ * caused an infinite loop. However we do not have to crash the kernel in order
+ * to assert if kernel has been fixed or not. All we have to do is to try to
+ * set a breakpoint, on any kernel address, then read it back and check if the
+ * value has been set or not.
+ *
* Kernel crash partially fixed in:
*
* commit f67b15037a7a50c57f72e69a6d59941ad90a0f0f
@@ -26,69 +35,54 @@
#include "tst_safe_stdio.h"
#if defined(__i386__) || defined(__x86_64__)
-#define SYMNAME_SIZE 256
-#define KERNEL_SYM "do_debug"
-static unsigned long break_addr;
static pid_t child_pid;
-static void setup(void)
-{
- int fcount;
- char endl, symname[256];
- FILE *fr = SAFE_FOPEN("/proc/kallsyms", "r");
-
- /* Find address of do_debug() in /proc/kallsyms */
- do {
- fcount = fscanf(fr, "%lx %*c %255s%c", &break_addr, symname,
- &endl);
-
- if (fcount <= 0 && feof(fr))
- break;
-
- if (fcount < 2) {
- fclose(fr);
- tst_brk(TBROK, "Unexpected data in /proc/kallsyms %d",
- fcount);
- }
-
- if (fcount >= 3 && endl != '\n')
- while (!feof(fr) && fgetc(fr) != '\n');
- } while (!feof(fr) && strcmp(symname, KERNEL_SYM));
-
- SAFE_FCLOSE(fr);
-
- if (strcmp(symname, KERNEL_SYM))
- tst_brk(TBROK, "Cannot find address of kernel symbol \"%s\"",
- KERNEL_SYM);
-
- if (!break_addr)
- tst_brk(TCONF, "Addresses in /proc/kallsyms are hidden");
+#if defined(__x86_64__)
+# define KERN_ADDR_MIN 0xffff800000000000
+# define KERN_ADDR_MAX 0xffffffffffffffff
+# define KERN_ADDR_BITS 64
+#elif defined(__i386__)
+# define KERN_ADDR_MIN 0xc0000000
+# define KERN_ADDR_MAX 0xffffffff
+# define KERN_ADDR_BITS 32
+#endif
- tst_res(TINFO, "Kernel symbol \"%s\" found at 0x%lx", KERNEL_SYM,
- break_addr);
-}
+static int deffered_check;
-static void debug_trap(void)
+static void setup(void)
{
- /* x86 instruction INT1 */
- asm volatile (".byte 0xf1");
+ /*
+ * When running in compat mode we can't pass 64 address to ptrace so we
+ * have to skip the test.
+ */
+ if (tst_kernel_bits() != KERN_ADDR_BITS)
+ tst_brk(TCONF, "Cannot pass 64bit kernel address in compat mode");
+
+
+ /*
+ * The original fix for the kernel haven't rejected the kernel address
+ * right away when breakpoint was modified from userspace it was
+ * disabled and the EINVAL was returned when dr7 was written to enable
+ * it again.
+ */
+ if (tst_kvercmp(4, 17, 0) < 0)
+ deffered_check = 1;
}
static void child_main(void)
{
raise(SIGSTOP);
- /* wait for SIGCONT from parent */
- debug_trap();
exit(0);
}
-static void run(void)
+static void ptrace_try_kern_addr(unsigned long kern_addr)
{
int status;
- pid_t child;
- child = child_pid = SAFE_FORK();
+ tst_res(TINFO, "Trying address 0x%lx", kern_addr);
+
+ child_pid = SAFE_FORK();
if (!child_pid)
child_main();
@@ -102,23 +96,46 @@ static void run(void)
SAFE_PTRACE(PTRACE_POKEUSER, child_pid,
(void *)offsetof(struct user, u_debugreg[7]), (void *)1);
- /* Return value intentionally ignored here */
- ptrace(PTRACE_POKEUSER, child_pid,
+ TEST(ptrace(PTRACE_POKEUSER, child_pid,
(void *)offsetof(struct user, u_debugreg[0]),
- (void *)break_addr);
+ (void *)kern_addr));
+
+ if (deffered_check) {
+ TEST(ptrace(PTRACE_POKEUSER, child_pid,
+ (void *)offsetof(struct user, u_debugreg[7]), (void *)1));
+ }
+
+ if (TST_RET != -1) {
+ tst_res(TFAIL, "ptrace() breakpoint with kernel addr succeeded");
+ } else {
+ if (TST_ERR == EINVAL) {
+ tst_res(TPASS | TTERRNO,
+ "ptrace() breakpoint with kernel addr failed");
+ } else {
+ tst_res(TFAIL | TTERRNO,
+ "ptrace() breakpoint on kernel addr should return EINVAL, got");
+ }
+ }
+
+ unsigned long addr;
+
+ addr = ptrace(PTRACE_PEEKUSER, child_pid,
+ (void*)offsetof(struct user, u_debugreg[0]), NULL);
+
+ if (!deffered_check && addr == kern_addr)
+ tst_res(TFAIL, "Was able to set breakpoint on kernel addr");
SAFE_PTRACE(PTRACE_DETACH, child_pid, NULL, NULL);
SAFE_KILL(child_pid, SIGCONT);
child_pid = 0;
+ tst_reap_children();
+}
- if (SAFE_WAITPID(child, &status, 0) != child)
- tst_brk(TBROK, "Received event from unexpected PID");
-
- if (!WIFSIGNALED(status))
- tst_brk(TBROK, "Received unexpected event from child");
-
- tst_res(TPASS, "Child killed by %s", tst_strsig(WTERMSIG(status)));
- tst_res(TPASS, "We're still here. Nothing bad happened, probably.");
+static void run(void)
+{
+ ptrace_try_kern_addr(KERN_ADDR_MIN);
+ ptrace_try_kern_addr(KERN_ADDR_MAX);
+ ptrace_try_kern_addr(KERN_ADDR_MIN + (KERN_ADDR_MAX - KERN_ADDR_MIN)/2);
}
static void cleanup(void)
@@ -133,7 +150,6 @@ static struct tst_test test = {
.setup = setup,
.cleanup = cleanup,
.forks_child = 1,
- .taint_check = TST_TAINT_W | TST_TAINT_D,
.tags = (const struct tst_tag[]) {
{"linux-git", "f67b15037a7a"},
{"CVE", "2018-1000199"},
--
2.26.2
Hi,
looks good and the test passes on older fixed kernels. Just one
compatibility issue below. Otherwise:
Reviewed-by: Martin Doucha <[email protected]>
On 04. 09. 20 16:09, Cyril Hrubis wrote:
> The original test was attempting to crash the kernel by setting a
> breakpoint on do_debug kernel function which, when triggered, caused an
> infinite loop in the kernel. The problem with this approach is that
> kernel internal function names are not stable at all and the name was
> changed recently, which made the test fail for no good reason.
>
> The original kernel fix made it however poissible to set a kernel
> address as a breakpoint and instead disabled the breakpoint on userspace
> modification. The error checks were deffered to write to the dr7 that
> enabled the breakpoint again.
>
> So on newer kernels we do not allow to set the breakpoint to the kernel
> addres at all, which means that the POKEUSR to dr0 has to fail with an
> address in a kernel range and also we read back the breakpoint address
> and check that it wasn't set just to be sure.
>
> On older kernels we check that the POKEUSER to dr7 that enables the
> breakpoint fails properly after the dr0 has been set to an address in
> the kernel range.
>
> Signed-off-by: Cyril Hrubis <[email protected]>
> CC: Andy Lutomirski <[email protected]>
> CC: Peter Zijlstra <[email protected]>
> CC: Thomas Gleixner <[email protected]>
> CC: Alexandre Chartre <[email protected]>
> ---
> testcases/kernel/syscalls/ptrace/ptrace08.c | 136 +++++++++++---------
> 1 file changed, 76 insertions(+), 60 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/ptrace/ptrace08.c b/testcases/kernel/syscalls/ptrace/ptrace08.c
> index 591aa0dd2..1b84ce376 100644
> --- a/testcases/kernel/syscalls/ptrace/ptrace08.c
> +++ b/testcases/kernel/syscalls/ptrace/ptrace08.c
> @@ -5,8 +5,17 @@
> *
> * CVE-2018-1000199
> *
> - * Test error handling when ptrace(POKEUSER) modifies debug registers.
> - * Even if the call returns error, it may create breakpoint in kernel code.
> + * Test error handling when ptrace(POKEUSER) modified x86 debug registers even
> + * when the call returned error.
> + *
> + * When the bug was present we could create breakpoint in the kernel code,
> + * which shoudn't be possible at all. The original CVE caused a kernel crash by
> + * setting a breakpoint on do_debug kernel function which, when triggered,
> + * caused an infinite loop. However we do not have to crash the kernel in order
> + * to assert if kernel has been fixed or not. All we have to do is to try to
> + * set a breakpoint, on any kernel address, then read it back and check if the
> + * value has been set or not.
> + *
> * Kernel crash partially fixed in:
> *
> * commit f67b15037a7a50c57f72e69a6d59941ad90a0f0f
> @@ -26,69 +35,54 @@
> #include "tst_safe_stdio.h"
>
> #if defined(__i386__) || defined(__x86_64__)
> -#define SYMNAME_SIZE 256
> -#define KERNEL_SYM "do_debug"
>
> -static unsigned long break_addr;
> static pid_t child_pid;
>
> -static void setup(void)
> -{
> - int fcount;
> - char endl, symname[256];
> - FILE *fr = SAFE_FOPEN("/proc/kallsyms", "r");
> -
> - /* Find address of do_debug() in /proc/kallsyms */
> - do {
> - fcount = fscanf(fr, "%lx %*c %255s%c", &break_addr, symname,
> - &endl);
> -
> - if (fcount <= 0 && feof(fr))
> - break;
> -
> - if (fcount < 2) {
> - fclose(fr);
> - tst_brk(TBROK, "Unexpected data in /proc/kallsyms %d",
> - fcount);
> - }
> -
> - if (fcount >= 3 && endl != '\n')
> - while (!feof(fr) && fgetc(fr) != '\n');
> - } while (!feof(fr) && strcmp(symname, KERNEL_SYM));
> -
> - SAFE_FCLOSE(fr);
> -
> - if (strcmp(symname, KERNEL_SYM))
> - tst_brk(TBROK, "Cannot find address of kernel symbol \"%s\"",
> - KERNEL_SYM);
> -
> - if (!break_addr)
> - tst_brk(TCONF, "Addresses in /proc/kallsyms are hidden");
> +#if defined(__x86_64__)
> +# define KERN_ADDR_MIN 0xffff800000000000
> +# define KERN_ADDR_MAX 0xffffffffffffffff
> +# define KERN_ADDR_BITS 64
> +#elif defined(__i386__)
> +# define KERN_ADDR_MIN 0xc0000000
> +# define KERN_ADDR_MAX 0xffffffff
> +# define KERN_ADDR_BITS 32
> +#endif
>
> - tst_res(TINFO, "Kernel symbol \"%s\" found at 0x%lx", KERNEL_SYM,
> - break_addr);
> -}
> +static int deffered_check;
>
> -static void debug_trap(void)
> +static void setup(void)
> {
> - /* x86 instruction INT1 */
> - asm volatile (".byte 0xf1");
> + /*
> + * When running in compat mode we can't pass 64 address to ptrace so we
> + * have to skip the test.
> + */
> + if (tst_kernel_bits() != KERN_ADDR_BITS)
> + tst_brk(TCONF, "Cannot pass 64bit kernel address in compat mode");
> +
> +
> + /*
> + * The original fix for the kernel haven't rejected the kernel address
> + * right away when breakpoint was modified from userspace it was
> + * disabled and the EINVAL was returned when dr7 was written to enable
> + * it again.
> + */
> + if (tst_kvercmp(4, 17, 0) < 0)
> + deffered_check = 1;
> }
>
> static void child_main(void)
> {
> raise(SIGSTOP);
> - /* wait for SIGCONT from parent */
> - debug_trap();
> exit(0);
> }
>
> -static void run(void)
> +static void ptrace_try_kern_addr(unsigned long kern_addr)
> {
> int status;
> - pid_t child;
>
> - child = child_pid = SAFE_FORK();
> + tst_res(TINFO, "Trying address 0x%lx", kern_addr);
> +
> + child_pid = SAFE_FORK();
>
> if (!child_pid)
> child_main();
> @@ -102,23 +96,46 @@ static void run(void)
> SAFE_PTRACE(PTRACE_POKEUSER, child_pid,
> (void *)offsetof(struct user, u_debugreg[7]), (void *)1);
>
> - /* Return value intentionally ignored here */
> - ptrace(PTRACE_POKEUSER, child_pid,
> + TEST(ptrace(PTRACE_POKEUSER, child_pid,
> (void *)offsetof(struct user, u_debugreg[0]),
> - (void *)break_addr);
> + (void *)kern_addr));
> +
> + if (deffered_check) {
> + TEST(ptrace(PTRACE_POKEUSER, child_pid,
> + (void *)offsetof(struct user, u_debugreg[7]), (void *)1));
> + }
> +
> + if (TST_RET != -1) {
> + tst_res(TFAIL, "ptrace() breakpoint with kernel addr succeeded");
> + } else {
> + if (TST_ERR == EINVAL) {
> + tst_res(TPASS | TTERRNO,
> + "ptrace() breakpoint with kernel addr failed");
> + } else {
> + tst_res(TFAIL | TTERRNO,
> + "ptrace() breakpoint on kernel addr should return EINVAL, got");
> + }
> + }
> +
> + unsigned long addr;
AFAICT, we're not compiling with --std=c99 so older compilers may
complain about the variable declaration here.
> +
> + addr = ptrace(PTRACE_PEEKUSER, child_pid,
> + (void*)offsetof(struct user, u_debugreg[0]), NULL);
> +
> + if (!deffered_check && addr == kern_addr)
> + tst_res(TFAIL, "Was able to set breakpoint on kernel addr");
>
> SAFE_PTRACE(PTRACE_DETACH, child_pid, NULL, NULL);
> SAFE_KILL(child_pid, SIGCONT);
> child_pid = 0;
> + tst_reap_children();
> +}
>
> - if (SAFE_WAITPID(child, &status, 0) != child)
> - tst_brk(TBROK, "Received event from unexpected PID");
> -
> - if (!WIFSIGNALED(status))
> - tst_brk(TBROK, "Received unexpected event from child");
> -
> - tst_res(TPASS, "Child killed by %s", tst_strsig(WTERMSIG(status)));
> - tst_res(TPASS, "We're still here. Nothing bad happened, probably.");
> +static void run(void)
> +{
> + ptrace_try_kern_addr(KERN_ADDR_MIN);
> + ptrace_try_kern_addr(KERN_ADDR_MAX);
> + ptrace_try_kern_addr(KERN_ADDR_MIN + (KERN_ADDR_MAX - KERN_ADDR_MIN)/2);
> }
>
> static void cleanup(void)
> @@ -133,7 +150,6 @@ static struct tst_test test = {
> .setup = setup,
> .cleanup = cleanup,
> .forks_child = 1,
> - .taint_check = TST_TAINT_W | TST_TAINT_D,
> .tags = (const struct tst_tag[]) {
> {"linux-git", "f67b15037a7a"},
> {"CVE", "2018-1000199"},
>
--
Martin Doucha [email protected]
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic
Hi!
> + /*
> + * The original fix for the kernel haven't rejected the kernel address
> + * right away when breakpoint was modified from userspace it was
> + * disabled and the EINVAL was returned when dr7 was written to enable
> + * it again.
> + */
> + if (tst_kvercmp(4, 17, 0) < 0)
> + deffered_check = 1;
And this should be 4.19 as the modify_user_hw_breakpoint_check() was made
uncoditional (by accident) in:
commit bd14406b78e6daa1ea3c1673bda1ffc9efdeead0
Author: Jiri Olsa <[email protected]>
Date: Mon Aug 27 11:12:25 2018 +0200
perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set
--
Cyril Hrubis
[email protected]
Hi!
> looks good and the test passes on older fixed kernels. Just one
> compatibility issue below.
I've fixed that and also the kernel version when the behavior had
changed and pushed, thanks for the review and testing.
...
> > + if (TST_RET != -1) {
> > + tst_res(TFAIL, "ptrace() breakpoint with kernel addr succeeded");
> > + } else {
> > + if (TST_ERR == EINVAL) {
> > + tst_res(TPASS | TTERRNO,
> > + "ptrace() breakpoint with kernel addr failed");
> > + } else {
> > + tst_res(TFAIL | TTERRNO,
> > + "ptrace() breakpoint on kernel addr should return EINVAL, got");
> > + }
> > + }
> > +
> > + unsigned long addr;
>
> AFAICT, we're not compiling with --std=c99 so older compilers may
> complain about the variable declaration here.
The default std for gcc has been at least gnu90 for ages, which includes
subset of c99 features as well including this one. So unless you
explicitly pass --std=c90 or older it will work just fine.
I've moved the declaration to the top of the function nevertheless.
--
Cyril Hrubis
[email protected]