Re: Possible bug in arch/i386/kernel/process.c for reloading of debug registers (DRx)?

Re: Possible bug in arch/i386/kernel/process.c for reloading of debug registers (DRx)?

[Andi Kleen]

Kevin Lawton <kevinlawton2001@yahoo.com> writes:

> I was scanning through the source and noticed the lines below.
> Should the code below, be reloading at least the local bits of
> DR7 if the current DR7 value != 0?  From a quick glance, it
> looks like only if the next task's DR7 value is non-zero,
> that DR7 is reloaded.  I'm wondering if this would leave
> a new task to receive "local" debug events for the previous
> task if prev->DR7!=0 && next->DR7==0.

The do_debug trap handler handles that. It checks that
the debug event is set in the current process before doing anything
and if they weren't they are clared.

So yes they leak, but only once and the user should never notice.

-Andi

Re: Possible bug in arch/i386/kernel/process.c for reloading of debug registers (DRx)?

[Pavel Machek]

Hi!

> > I was scanning through the source and noticed the lines below.
> > Should the code below, be reloading at least the local bits of
> > DR7 if the current DR7 value != 0?  From a quick glance, it
> > looks like only if the next task's DR7 value is non-zero,
> > that DR7 is reloaded.  I'm wondering if this would leave
> > a new task to receive "local" debug events for the previous
> > task if prev->DR7!=0 && next->DR7==0.
> 
> The do_debug trap handler handles that. It checks that
> the debug event is set in the current process before doing anything
> and if they weren't they are clared.
> 
> So yes they leak, but only once and the user should never notice.

What if DRx contains sensitive data? ...Its probably pretty
unlikely. Still it allows for example easy communication between tasks
that should not be able to communicate.

								Pavel

-- 
Worst form of spam? Adding advertisment signatures ala sourceforge.net.
What goes next? Inserting advertisment *into* email?

Re: Possible bug in arch/i386/kernel/process.c for reloading of debug registers (DRx)?

[Andi Kleen]

> What if DRx contains sensitive data? ...Its probably pretty
> unlikely. Still it allows for example easy communication between tasks
> that should not be able to communicate.

The user never sees the stale value, it is eaten by the kernel's do_debug
handler.

-Andi

Re: Possible bug in arch/i386/kernel/process.c for reloading of debug registers (DRx)?

[Jamie Lokier]

Andi Kleen wrote:
> > What if DRx contains sensitive data? ...Its probably pretty
> > unlikely. Still it allows for example easy communication between tasks
> > that should not be able to communicate.
> 
> The user never sees the stale value, it is eaten by the kernel's do_debug
> handler.

DR6 isn't cleared.  Here is a nice security exploit for you:

	- Task A sets DR0 and DR7 to enable a watchpoint (or breakpoint).
	- It also clears DR6.
	- Task A wakes up task B, which has DR7 clear.
	- Task A then communicates with "sshd" or some other sensitive task.

	- Because of lazy DR7 clearing, sshd inherits the watchpoints.
	- If sshd reads the memory address mentioned in DR0, it will
	  call do_debug in the kernel, which clears DR7 and continues.
	- However, DR6 bit B0 is now set.

	- Eventually task B is scheduled.  It inherits the value of DR6
	  from sshd, and therefore knows if sshd read from a particular
	  memory location.

	- Task A and task B cooperate to analyse what values sshd is
	  examining in its lookup tables, and therefore retrieve the
	  server key or something.  (Hand waving at this point).

-- Jamie

Re: Possible bug in arch/i386/kernel/process.c for reloading of debug registers (DRx)?

[Andi Kleen]

> 	- However, DR6 bit B0 is now set.

You cannot detect it. Linux offers no way to read DR6 from user space
as far as I can see. The only way to handle break points is to catch
the signals caused by the debug exceptions.

Yo access debug registers you need to use ptrace from another process.
ptrace only ever returns cached values in tsk->thread, but the register is 
never stored in there.

So in fact __switch_to could drop the loaddebug(next, 6) because it is 
useless.

-Andi

[PATCH] Optimisation and CONFIG_PREEMPT fix of reloading of debug registers

[Jamie Lokier]

Andi Kleen wrote:
> > 	- However, DR6 bit B0 is now set.
> 
> You cannot detect it. Linux offers no way to read DR6 from user space
> as far as I can see. The only way to handle break points is to catch
> the signals caused by the debug exceptions.
> 
> Yo access debug registers you need to use ptrace from another process.
> ptrace only ever returns cached values in tsk->thread, but the register is 
> never stored in there.

To be precise, DR6 is stored in tsk->thread, in do_debug, but only
when DR7 is non-zero.  So there is no information leak.

> So in fact __switch_to could drop the loaddebug(next, 6) because it is 
> useless.

If you remove it, observable behaviour will change.  The bits set in
DR6 are cumulative over multiple debug traps.  Without the
loaddebug(next, 6), that accumulation is lost.  GDB won't mind because
it always uses the value of DR6 immediately after a debug trap and
before resuming a thread, but some other program could use the value
differently.

If you go ahead and remove the loaddebug(next, 6), it would make sense
to also clear DR6 in do_debug after reading it, so that programs see a
consistent value in debugreg[6] - i.e. only the bits set from the last
debug trap.

Also, it is essential that DR6 is cleared _somewhere_ before switching
to a task which has DR7 set, because otherwise you have the following
rather subtle hole:

    - Exploit program forks, parent attaches to child and stops child.
    - Parent sets DR3 in child to probe address, and DR7 to enable
      desired kind of breakpoint or watchpoint.  Also clears DR6.
    - Parent wakes up child and waits for child to stop.

          => Child schedules and DR3+DR6+DR7 are all loaded.

    - Child sends itself SIGSTOP, which wakes up parent.
    - Parent clears DR7 in child, wakes up child and waits for it to stop.

          => Child schedules, debug registers remain loaded
             although no task has debugreg[7] set.

    - Child interacts with subject process (e.g. "sshd") and sleeps
      for long enough to ensure subject process gets a chance to run.

          => sshd process triggers debug trap, clears DR7 but not DR6.

    - Child finished sleeping and sends itself SIGSTOP, which wakes parent.
    - Parent sets DR7 in child, wakes up child and waits for it to trap.

          => debugreg[7] has a non-zero value in the child
          => DR6 maintains its value from the sshd process trap
             (This is all assuming you removed the loaddebug(next, 6))

    - Child executes "int $1", which calls do_debug in the kernel.
    - Kernel notes that "condition & DR_TRAP0" is set (from DR6),
      and current->thread.debugreg[7] is non-zero (recently set by parent).
    - So kernel stores current value of DR6 in child's debugreg[6].
    - Parent is woken by the child's SIGTRAP, and reads child's debugreg[6].

          => Parent has read DR6 from the sshd process trap.
          => Potentially bad information leakage.

The outcome of these stories is:

    1. Yes you can remove loaddebug(next, 6) from switch_to.
    2. But then you _must_ unconditionally clear DR6 immediately after
       reading it in do_debug().
    3. DR6 must be clear when initialising each CPU.  Fortunately this
       is already done in cpu_init().
    4. If you change the line which saves DR6 into debugreg[6] to use
       "|=" instead of "=", you will even be able to preserve the currently
       observable behaviour of accumulating bits in debugreg[6] over
       multiple debug traps.
    5. I hope there is no SMP race condition where it is possible for
       ptrace(PTRACE_POKEUSER, ...) to succeed on a running process,
       even briefly.  That would trick the logic in do_debug() into
       revealing DR6 (with or without changes 1..4.)

Oh, and:

    6. If CONFIG_PREEMPT is enabled, and the kernel is preempted
       just after the attacked process hits the debug condition,
       but before do_debug() has a chance to run, changes 1..4
       introduce an information leak.  So the debug trap must be
       changed to an interrupt gate.

Point 6 is a bug in the current kernel.  DR6 could be read from the
wrong CPU if preempted.  It is exactly the same problem as reading
%cr2 in the page fault handler.

Patch is attached.  Untested.  Enjoy :)

-- Jamie

diff -urN --exclude='*.o' --exclude='.??*' --exclude=asm --exclude='vmlinux*' --exclude=System.map --exclude=bzImage orig-2.5.59/arch/i386/kernel/process.c dr6-2.5.59/arch/i386/kernel/process.c
--- orig-2.5.59/arch/i386/kernel/process.c	2003-02-09 05:49:12.000000000 +0000
+++ dr6-2.5.59/arch/i386/kernel/process.c	2003-02-09 06:03:44.000000000 +0000
@@ -467,8 +467,7 @@
 		loaddebug(next, 1);
 		loaddebug(next, 2);
 		loaddebug(next, 3);
-		/* no 4 and 5 */
-		loaddebug(next, 6);
+		/* No 4 and 5, and 6 always contains zero (see do_debug()). */
 		loaddebug(next, 7);
 	}
 
diff -urN --exclude='*.o' --exclude='.??*' --exclude=asm --exclude='vmlinux*' --exclude=System.map --exclude=bzImage orig-2.5.59/arch/i386/kernel/traps.c dr6-2.5.59/arch/i386/kernel/traps.c
--- orig-2.5.59/arch/i386/kernel/traps.c	2003-02-09 05:49:56.000000000 +0000
+++ dr6-2.5.59/arch/i386/kernel/traps.c	2003-02-09 06:10:21.000000000 +0000
@@ -514,6 +514,14 @@
 
 	__asm__ __volatile__("movl %%db6,%0" : "=r" (condition));
 
+	/* Unconditionally clear DR6 to prevent information leaks
+	   due to lazy DR7 setting. */
+	__asm__ __volatile__("movl %0,%%db6" : /* no output */ : "r" (0));
+
+	/* It's safe to allow irq's after DR6 has been saved */
+	if (regs->eflags & X86_EFLAGS_IF)
+		local_irq_enable();
+
 	/* Mask out spurious debug traps due to lazy DR7 setting */
 	if (condition & (DR_TRAP0|DR_TRAP1|DR_TRAP2|DR_TRAP3)) {
 		if (!tsk->thread.debugreg[7])
@@ -523,8 +531,9 @@
 	if (regs->eflags & VM_MASK)
 		goto debug_vm86;
 
-	/* Save debug status register where ptrace can see it */
-	tsk->thread.debugreg[6] = condition;
+	/* Save debug status register where ptrace can see it.  Bits are
+	   ORed into the stored DR6, just like the CPU does with real DR6. */
+	tsk->thread.debugreg[6] |= condition;
 
 	/* Mask out spurious TF errors due to lazy TF clearing */
 	if (condition & DR_STEP) {
@@ -831,7 +840,7 @@
 #endif
 
 	set_trap_gate(0,&divide_error);
-	set_trap_gate(1,&debug);
+	set_intr_gate(1,&debug);
 	set_intr_gate(2,&nmi);
 	set_system_gate(3,&int3);	/* int3-5 can be called from all */
 	set_system_gate(4,&overflow);