In 2.2.21-pre1 some vm86 bugs have been attempted fixed.
The bug did become smaller, but unfortunately didn't get
completely fixed.
The problem was in the kernels access to the vm86 mode
stack. This would previously lead to an Oops if the
memory was not mapped, or was mapped with insufficient
permitions.
The patch adds the necesarry memory checks and returns
VM86_UNKNOWN on error. But at this point the vm86 IP
register will already have stepped over the offending
instruction, on return the IP register should point to
the offending instruction.
In addition to that there were doubts about the
correctness of the return value. For most emulated
instructions the solution most consistent with ordinary
instructions accessing the stack would be to send a
SIGSEGV to the process. But I guess that would be more
difficult both for the kernel code and userspace code.
I actually don't like the idea of code in virtual 86
mode producing SIGSEGV's, it complicates the handling
in userspace when you never know if the SIGSEGV is
caused in virtual 86 mode or protected mode. If all
these SIGSEGV's from virtual 86 mode could be changed
into a return value from vm86() the SIGSEGV handler
could be avoided which will make debuging of the user
code easier. Bottom line of that is that for all those
cases I like the VM86_UNKNOWN return value, but a new
one like VM86_SIGSEGV would IMHO be better.
For the do_int instruction the situation is slightly
different, I don't think VM86_UNKNOWN is a good idea
here. A better idea would be VM86_INTx + (i << 8)
which can be returned for so many different reasons
already that the userspace code can be expected to
handle it. This also avoids the need to step back IP
which would be difficult in this case, since we
actually don't know the instruction.
I have also spotted another bug in the two
set_vflags functions, in both cases two lines are
missing:
else
clear_IF(regs);
Also the patch only seems to have been applied to
2.2.x, the Oops still happens in 2.4.18. I will be
back with a patch for 2.4.18 once I have resolved
the mentioned problems.
Any comments?
--
Kasper Dupont -- der bruger for meget tid p? usenet.
For sending spam use mailto:[email protected]
diff -Nur linux.old/arch/i386/kernel/vm86.c linux.new/arch/i386/kernel/vm86.c
--- linux.old/arch/i386/kernel/vm86.c Fri Mar 22 12:53:37 2002
+++ linux.new/arch/i386/kernel/vm86.c Fri Mar 22 14:25:08 2002
@@ -3,6 +3,13 @@
*
* Copyright (C) 1994 Linus Torvalds
*/
+
+/*
+ * Bugfixes Copyright 2002 by Manfred Spraul and
+ * Kasper Dupont <[email protected]>
+ *
+ */
+
#include <linux/errno.h>
#include <linux/sched.h>
#include <linux/kernel.h>
@@ -290,12 +297,25 @@
regs->eflags &= ~TF_MASK;
}
+/* It is correct to call set_IF(regs) from the set_vflags_*
+ * functions. However someone forgot to call clear_IF(regs)
+ * in the opposite case.
+ * After the command sequence CLI PUSHF STI POPF you should
+ * end up with interrups disabled, but you ended up with
+ * interrupts enabled.
+ * ( I was testing my own changes, but the only bug I
+ * could find was in a function I had not changed. )
+ * [KD]
+ */
+
static inline void set_vflags_long(unsigned long eflags, struct kernel_vm86_regs * regs)
{
set_flags(VEFLAGS, eflags, current->thread.v86mask);
set_flags(regs->eflags, eflags, SAFE_MASK);
if (eflags & IF_MASK)
set_IF(regs);
+ else
+ clear_IF(regs);
}
static inline void set_vflags_short(unsigned short flags, struct kernel_vm86_regs * regs)
@@ -304,6 +324,8 @@
set_flags(regs->eflags, flags, SAFE_MASK);
if (flags & IF_MASK)
set_IF(regs);
+ else
+ clear_IF(regs);
}
static inline unsigned long get_vflags(struct kernel_vm86_regs * regs)
@@ -327,75 +349,184 @@
* Boy are these ugly, but we need to do the correct 16-bit arithmetic.
* Gcc makes a mess of it, so we do it inline and use non-obvious calling
* conventions..
+ * FIXME: is VM86_UNKNOWN really the correct return code? [MS??]
+ * No that wasn't correct, it depends on the context, so lets
+ * make it an argument to the macro. [KD]
+ */
+#define pushb(base, ptr, val, regs, errcode) \
+ do { \
+ int err; \
+ __asm__ __volatile__( \
+ "decw %w0\n\t" \
+ "1: movb %3,0(%2,%0)\n\t" \
+ "xor %1,%1\n\t" \
+ "2:\n" \
+ ".section .fixup,\"ax\"\n\t" \
+ "3: movl $1,%1\n\t" \
+ " jmp 2b\n\t" \
+ ".previous\n" \
+ ".section __ex_table,\"a\"\n" \
+ " .align 4\n" \
+ " .long 1b,3b\n" \
+ ".previous" \
+ : "=r" (ptr), "=r" (err) \
+ : "r" (base), "q" (val), "0" (ptr)); \
+ if (err) \
+ return_to_32bit(regs, errcode); \
+ } while(0)
+
+#define pushw(base, ptr, val, regs, errcode) \
+ do { \
+ int err; \
+ __asm__ __volatile__( \
+ "decw %w0\n\t" \
+ "1: movb %h3,0(%2,%0)\n\t" \
+ "decw %w0\n\t" \
+ "2: movb %b3,0(%2,%0)\n\t" \
+ "xor %1,%1\n\t" \
+ "3:\n" \
+ ".section .fixup,\"ax\"\n\t" \
+ "4: movl $1,%1\n\t" \
+ " jmp 3b\n\t" \
+ ".previous\n" \
+ ".section __ex_table,\"a\"\n" \
+ " .align 4\n" \
+ " .long 1b,4b\n" \
+ " .long 2b,4b\n" \
+ ".previous" \
+ : "=r" (ptr), "=r" (err) \
+ : "r" (base), "q" (val), "0" (ptr)); \
+ if (err) \
+ return_to_32bit(regs, errcode); \
+ } while(0)
+
+#define pushl(base, ptr, val, regs, errcode) \
+ do { \
+ int err; \
+ __asm__ __volatile__( \
+ "decw %w0\n\t" \
+ "rorl $16,%3\n\t" \
+ "1: movb %h3,0(%2,%0)\n\t" \
+ "decw %w0\n\t" \
+ "2: movb %b3,0(%2,%0)\n\t" \
+ "decw %w0\n\t" \
+ "rorl $16,%3\n\t" \
+ "3: movb %h3,0(%2,%0)\n\t" \
+ "decw %w0\n\t" \
+ "4: movb %b3,0(%2,%0)\n\t" \
+ "xor %1,%1\n\t" \
+ "5:\n" \
+ ".section .fixup,\"ax\"\n\t" \
+ "6: movl $1,%1\n\t" \
+ " jmp 5b\n\t" \
+ ".previous\n" \
+ ".section __ex_table,\"a\"\n" \
+ " .align 4\n" \
+ " .long 1b,6b\n" \
+ " .long 2b,6b\n" \
+ " .long 3b,6b\n" \
+ " .long 4b,6b\n" \
+ ".previous" \
+ : "=r" (ptr), "=r" (err) \
+ : "r" (base), "q" (val), "0" (ptr)); \
+ if (err) \
+ return_to_32bit(regs, errcode); \
+ } while(0)
+
+#define popb(base, ptr, regs, errcode) \
+ ({ \
+ unsigned long __res; \
+ unsigned int err; \
+ __asm__ __volatile__( \
+ "1:movb 0(%1,%0),%b2\n\t" \
+ "incw %w0\n\t" \
+ "xor %3,%3\n\t" \
+ "2:\n" \
+ ".section .fixup,\"ax\"\n\t" \
+ "3: movl $1,%1\n\t" \
+ " jmp 2b\n\t" \
+ ".previous\n" \
+ ".section __ex_table,\"a\"\n" \
+ " .align 4\n" \
+ " .long 1b,3b\n" \
+ ".previous" \
+ : "=r" (ptr), "=r" (base), "=q" (__res), \
+ "=r" (err) \
+ : "0" (ptr), "1" (base), "2" (0)); \
+ if (err) \
+ return_to_32bit(regs, errcode); \
+ __res; \
+ })
+
+#define popw(base, ptr, regs, errcode) \
+ ({ \
+ unsigned long __res; \
+ unsigned int err; \
+ __asm__ __volatile__( \
+ "1:movb 0(%1,%0),%b2\n\t" \
+ "incw %w0\n\t" \
+ "2:movb 0(%1,%0),%h2\n\t" \
+ "incw %w0\n\t" \
+ "xor %3,%3\n\t" \
+ "3:\n" \
+ ".section .fixup,\"ax\"\n\t" \
+ "4: movl $1,%1\n\t" \
+ " jmp 3b\n\t" \
+ ".previous\n" \
+ ".section __ex_table,\"a\"\n" \
+ " .align 4\n" \
+ " .long 1b,4b\n" \
+ " .long 2b,4b\n" \
+ ".previous" \
+ : "=r" (ptr), "=r" (base), "=q" (__res), \
+ "=r" (err) \
+ : "0" (ptr), "1" (base), "2" (0)); \
+ if (err) \
+ return_to_32bit(regs, errcode); \
+ __res; \
+ })
+
+#define popl(base, ptr, regs, errcode) \
+ ({ \
+ unsigned long __res; \
+ unsigned int err; \
+ __asm__ __volatile__( \
+ "1:movb 0(%1,%0),%b2\n\t" \
+ "incw %w0\n\t" \
+ "2:movb 0(%1,%0),%h2\n\t" \
+ "incw %w0\n\t" \
+ "rorl $16,%2\n\t" \
+ "3:movb 0(%1,%0),%b2\n\t" \
+ "incw %w0\n\t" \
+ "4:movb 0(%1,%0),%h2\n\t" \
+ "incw %w0\n\t" \
+ "rorl $16,%2\n\t" \
+ "xor %3,%3\n\t" \
+ "5:\n" \
+ ".section .fixup,\"ax\"\n\t" \
+ "6: movl $1,%1\n\t" \
+ " jmp 5b\n\t" \
+ ".previous\n" \
+ ".section __ex_table,\"a\"\n" \
+ " .align 4\n" \
+ " .long 1b,6b\n" \
+ " .long 2b,6b\n" \
+ " .long 3b,6b\n" \
+ " .long 4b,6b\n" \
+ ".previous" \
+ : "=r" (ptr), "=r" (base), "=q" (__res), \
+ "=r" (err) \
+ : "0" (ptr), "1" (base), "2" (0)); \
+ if (err) \
+ return_to_32bit(regs, errcode); \
+ __res; \
+ })
+
+/* There are so many possible reasons for this function to return
+ * VM86_INTx, so adding another doesn't bother me. We can expect
+ * userspace programs to be able to handle it. (Getting a problem
+ * in userspace is always better than an Oops anyway.) [KD]
*/
-#define pushb(base, ptr, val) \
-__asm__ __volatile__( \
- "decw %w0\n\t" \
- "movb %2,0(%1,%0)" \
- : "=r" (ptr) \
- : "r" (base), "q" (val), "0" (ptr))
-
-#define pushw(base, ptr, val) \
-__asm__ __volatile__( \
- "decw %w0\n\t" \
- "movb %h2,0(%1,%0)\n\t" \
- "decw %w0\n\t" \
- "movb %b2,0(%1,%0)" \
- : "=r" (ptr) \
- : "r" (base), "q" (val), "0" (ptr))
-
-#define pushl(base, ptr, val) \
-__asm__ __volatile__( \
- "decw %w0\n\t" \
- "rorl $16,%2\n\t" \
- "movb %h2,0(%1,%0)\n\t" \
- "decw %w0\n\t" \
- "movb %b2,0(%1,%0)\n\t" \
- "decw %w0\n\t" \
- "rorl $16,%2\n\t" \
- "movb %h2,0(%1,%0)\n\t" \
- "decw %w0\n\t" \
- "movb %b2,0(%1,%0)" \
- : "=r" (ptr) \
- : "r" (base), "q" (val), "0" (ptr))
-
-#define popb(base, ptr) \
-({ unsigned long __res; \
-__asm__ __volatile__( \
- "movb 0(%1,%0),%b2\n\t" \
- "incw %w0" \
- : "=r" (ptr), "=r" (base), "=q" (__res) \
- : "0" (ptr), "1" (base), "2" (0)); \
-__res; })
-
-#define popw(base, ptr) \
-({ unsigned long __res; \
-__asm__ __volatile__( \
- "movb 0(%1,%0),%b2\n\t" \
- "incw %w0\n\t" \
- "movb 0(%1,%0),%h2\n\t" \
- "incw %w0" \
- : "=r" (ptr), "=r" (base), "=q" (__res) \
- : "0" (ptr), "1" (base), "2" (0)); \
-__res; })
-
-#define popl(base, ptr) \
-({ unsigned long __res; \
-__asm__ __volatile__( \
- "movb 0(%1,%0),%b2\n\t" \
- "incw %w0\n\t" \
- "movb 0(%1,%0),%h2\n\t" \
- "incw %w0\n\t" \
- "rorl $16,%2\n\t" \
- "movb 0(%1,%0),%b2\n\t" \
- "incw %w0\n\t" \
- "movb 0(%1,%0),%h2\n\t" \
- "incw %w0\n\t" \
- "rorl $16,%2" \
- : "=r" (ptr), "=r" (base), "=q" (__res) \
- : "0" (ptr), "1" (base)); \
-__res; })
-
static void do_int(struct kernel_vm86_regs *regs, int i, unsigned char * ssp, unsigned long sp)
{
unsigned long *intr_ptr, segoffs;
@@ -411,9 +542,9 @@
goto cannot_handle;
if ((segoffs >> 16) == BIOSSEG)
goto cannot_handle;
- pushw(ssp, sp, get_vflags(regs));
- pushw(ssp, sp, regs->cs);
- pushw(ssp, sp, IP(regs));
+ pushw(ssp, sp, get_vflags(regs), regs, VM86_INTx + (i << 8));
+ pushw(ssp, sp, regs->cs, regs, VM86_INTx + (i << 8));
+ pushw(ssp, sp, IP(regs), regs, VM86_INTx + (i << 8));
regs->cs = segoffs >> 16;
SP(regs) -= 6;
IP(regs) = segoffs & 0xffff;
@@ -448,6 +579,22 @@
return 0;
}
+/* I guess the most consistent with other stack access like
+ * PUSH AX and similar would be a SIGSEGV, but I really don't
+ * like that so I will just stick to Manfred's solution with
+ * a return code. The SIGSEGV would be harder to implement
+ * here, and also more difficult for userspace code to handle.
+ * I would prefer a new return code, but in order not to mess
+ * up unsuspecting applications I will not invent a new code
+ * yet. Either way we get the problem moved from kernel space
+ * to user space, which is one step in the right direction.
+ * Some existing user space code might even be ready to deal
+ * with VM86_UNKNOWN, since handle_vm86_fault can return that
+ * for so many other reasons as well. I have also fixed the
+ * problem with incorrect IP by moving the increment after the
+ * actual execution of the instruction. [KD]
+ */
+#define VM86_SIGSEGV VM86_UNKNOWN
void handle_vm86_fault(struct kernel_vm86_regs * regs, long error_code)
{
unsigned char *csp, *ssp;
@@ -455,7 +602,7 @@
#define CHECK_IF_IN_TRAP \
if (VMPI.vm86dbg_active && VMPI.vm86dbg_TFpendig) \
- pushw(ssp,sp,popw(ssp,sp) | TF_MASK);
+ pushw(ssp,sp,popw(ssp,sp, regs, VM86_SIGSEGV) | TF_MASK, regs, VM86_SIGSEGV);
#define VM86_FAULT_RETURN \
if (VMPI.force_return_for_pic && (VEFLAGS & (IF_MASK | VIF_MASK))) \
return_to_32bit(regs, VM86_PICRETURN); \
@@ -466,35 +613,39 @@
sp = SP(regs);
ip = IP(regs);
- switch (popb(csp, ip)) {
+ switch (popb(csp, ip, regs, VM86_SIGSEGV)) {
/* operand size override */
case 0x66:
- switch (popb(csp, ip)) {
+ switch (popb(csp, ip, regs, VM86_SIGSEGV)) {
/* pushfd */
case 0x9c:
+ pushl(ssp, sp, get_vflags(regs), regs, VM86_SIGSEGV);
SP(regs) -= 4;
IP(regs) += 2;
- pushl(ssp, sp, get_vflags(regs));
VM86_FAULT_RETURN;
/* popfd */
case 0x9d:
+ CHECK_IF_IN_TRAP
+ set_vflags_long(popl(ssp, sp, regs, VM86_SIGSEGV), regs);
SP(regs) += 4;
IP(regs) += 2;
- CHECK_IF_IN_TRAP
- set_vflags_long(popl(ssp, sp), regs);
VM86_FAULT_RETURN;
/* iretd */
case 0xcf:
- SP(regs) += 12;
- IP(regs) = (unsigned short)popl(ssp, sp);
- regs->cs = (unsigned short)popl(ssp, sp);
+ {
+ unsigned long newip=popl(ssp, sp, regs, VM86_SIGSEGV);
+ unsigned long newcs=popl(ssp, sp, regs, VM86_SIGSEGV);
CHECK_IF_IN_TRAP
- set_vflags_long(popl(ssp, sp), regs);
+ set_vflags_long(popl(ssp, sp, regs, VM86_SIGSEGV), regs);
+ SP(regs) += 12;
+ IP(regs) = (unsigned short)newip;
+ regs->cs = (unsigned short)newcs;
VM86_FAULT_RETURN;
+ }
/* need this to avoid a fallthrough */
default:
return_to_32bit(regs, VM86_UNKNOWN);
@@ -502,22 +653,22 @@
/* pushf */
case 0x9c:
+ pushw(ssp, sp, get_vflags(regs), regs, VM86_SIGSEGV);
SP(regs) -= 2;
IP(regs)++;
- pushw(ssp, sp, get_vflags(regs));
VM86_FAULT_RETURN;
/* popf */
case 0x9d:
+ CHECK_IF_IN_TRAP
+ set_vflags_short(popw(ssp, sp, regs, VM86_SIGSEGV), regs);
SP(regs) += 2;
IP(regs)++;
- CHECK_IF_IN_TRAP
- set_vflags_short(popw(ssp, sp), regs);
VM86_FAULT_RETURN;
/* int xx */
case 0xcd: {
- int intno=popb(csp, ip);
+ int intno=popb(csp, ip, regs, VM86_SIGSEGV);
IP(regs) += 2;
if (VMPI.vm86dbg_active) {
if ( (1 << (intno &7)) & VMPI.vm86dbg_intxxtab[intno >> 3] )
@@ -529,12 +680,16 @@
/* iret */
case 0xcf:
- SP(regs) += 6;
- IP(regs) = popw(ssp, sp);
- regs->cs = popw(ssp, sp);
+ {
+ unsigned short newip=popw(ssp, sp, regs, VM86_SIGSEGV);
+ unsigned short newcs=popw(ssp, sp, regs, VM86_SIGSEGV);
CHECK_IF_IN_TRAP
- set_vflags_short(popw(ssp, sp), regs);
+ set_vflags_short(popw(ssp, sp, regs, VM86_SIGSEGV), regs);
+ SP(regs) += 6;
+ IP(regs) = newip;
+ regs->cs = newcs;
VM86_FAULT_RETURN;
+ }
/* cli */
case 0xfa: