From owner-freebsd-current@FreeBSD.ORG Sat Nov 5 22:31:07 2011 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 78A261065674 for ; Sat, 5 Nov 2011 22:31:07 +0000 (UTC) (envelope-from rysto32@gmail.com) Received: from mail-ww0-f50.google.com (mail-ww0-f50.google.com [74.125.82.50]) by mx1.freebsd.org (Postfix) with ESMTP id 0DB818FC17 for ; Sat, 5 Nov 2011 22:31:06 +0000 (UTC) Received: by wwp14 with SMTP id 14so5164734wwp.31 for ; Sat, 05 Nov 2011 15:31:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:date:message-id:subject:from:to:content-type; bh=yp+iQkKlVFookDn83T0YExW24NBXQHid7ntXtQm57zc=; b=Y0kXUAHDuQ+BDCPYG4Om3Q7DwOrvSBN6zDowXIpY97QSTniWdpCYv9Lq1Ou+crf0Ie CuWQ1RFDopOeGDHGhUzQriFA/ga8p4ngLqD416e2nFgFm1FAAyugFMaZzd07BzWghCu+ K2dEkfArE+uXgvSjIzJ7XLNAVhkEPAJlPhL5A= MIME-Version: 1.0 Received: by 10.181.13.82 with SMTP id ew18mr3751006wid.16.1320532265797; Sat, 05 Nov 2011 15:31:05 -0700 (PDT) Received: by 10.180.8.34 with HTTP; Sat, 5 Nov 2011 15:31:05 -0700 (PDT) Date: Sat, 5 Nov 2011 18:31:05 -0400 Message-ID: From: Ryan Stone To: FreeBSD Current Content-Type: text/plain; charset=ISO-8859-1 Subject: [PATCH] Fix kernel panics when using dtrace fbt return probes on i386 X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 05 Nov 2011 22:31:07 -0000 I have a patch that fixes crash when using dtrace fbt return probes on i386. dtrace implements an fbt probe by overwriting a small part of the function when the probe is active. On i386, it writes an invalid opcode. dtrace has a hook into the invalid opcode fault handler that checks whether the fault was due to an active probe; if it was, the probe fires and then the fault handler emulates the behaviour of the instruction that was replaced with the invalid opcode and returns control to the instruction after the invalid opcode. The bug is in the emulation of the leave instruction(which is replaced for an fbt return probe). The is a small window in which the emulation leaves critical state above the current stack pointer. If the CPU takes an interrupt in this window, the trap frame corrupts this state. The leave instruction is used to pop off a stack frame immediately prior to returning from a function. Because the leave emulation is running in a fault handler, it must copy the trap frame to the bottom of the stack frame that is being removed, set its stack pointer to the start of the new trap frame and then execute the iret instruction. There are actually two bugs in the current implementation. The first is that immediately before restoring the stack pointer to point to the new trap frame, the emulation must save the new stack pointer on the stack, restore the values of its scratch registers, and then load the new stack pointer back from the stack. The current implementation saves the new stack pointer at -4(%esp). If an interrupt is taking after the new stack pointer is saved at this location the trap frame will overwrite the new stack pointer. The fix for this is to instead save the new stack pointer at 8(%esp), which was part of the old trap frame that was copied forward. This is safe as we know that the trap frame must exist (so 8(%esp) can't possibly point to still-valid data from the next stack frame, for instances) and at this point we have copied the stack frame forward so we can safely overwrite the old one without any issues. The second bug is that when the new stack pointer is restored, it does not point at the new trap frame. Instead it points 8 bytes into the trap frame. The emulation code corrects for this by subtracting 8 from %esp before executing the iret. However if we take an interrupt after we have restored the new stack pointer, but before subtracting 8 from %esp, the trap frame from the interrupt will overwrite the first 8 bytes of the invalid opcode trap frame, causing us to crash when we execute the iret. The fix is to adjust the new stack pointer before saving onto the stack in the first place, so that we we restore it it is immediately valid, closing the window in which an interrupt can corrupt anything. If there are no objections to this patch I will commit it soonish. I would appreciate some review, but IMO this really needs to get in for 9.0 as a central tenet of dtrace is that it will not crash your system. Index: sys/cddl/dev/dtrace/i386/dtrace_asm.S =================================================================== --- sys/cddl/dev/dtrace/i386/dtrace_asm.S (revision 227094) +++ sys/cddl/dev/dtrace/i386/dtrace_asm.S (working copy) @@ -125,11 +125,11 @@ movl 8(%esp), %eax /* load calling EIP */ incl %eax /* increment over LOCK prefix */ movl %eax, -8(%ebx) /* store calling EIP */ - movl %ebx, -4(%esp) /* temporarily store new %esp */ + subl $8, %ebx /* adjust for three pushes, one pop */ + movl %ebx, 8(%esp) /* temporarily store new %esp */ popl %ebx /* pop off temp */ popl %eax /* pop off temp */ - movl -12(%esp), %esp /* set stack pointer */ - subl $8, %esp /* adjust for three pushes, one pop */ + movl (%esp), %esp /* set stack pointer */ iret /* return from interrupt */ invop_nop: /*