lkml.org 
[lkml]   [2004]   [Sep]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: ESP corruption bug - what CPUs are affected?
On Wed, Sep 22, 2004 at 10:49:45PM +0400, Stas Sergeev wrote:
> Hi Petr et al.
>
> I coded up something that remotely looks like the
> discussed patch.
> I have deviated from your proposals at some important
> points:
> 1. I am not allocating the ring1 stack separately,
> I am allocating it on a ring0 stack. Much simpler
> code, although non-reentrant/preempt-unsafe.

I think that it is problem. And do not forget that NMI can occur at any
place, so I'm not quite sure how your code works.

> 2. I am disabling the interrupts after all. That's
> because of the preempt-unsafeness. I pass up the
> IOPL=1 when necessary, to avoid problems.
> But I guess also with your technique the interrupts
> had to be disabled, unless the ring1 stack is
> per-thread.

What if exception occurs in CPL1 code? Processor reloads SS/ESP from
TSS, pointing at top of CPL0 stack - where it overwrites CPL1 stack -
- and it pushes on top of CPL0 stack address of CPL1 - and real return
address for CPL3 was lost :-( Boom.

You change base address for SS segment - in that case you need per-CPU
SS segment, it wont' work with global SS segment.

> 3. I am using LAR. Do you really think it can be
> slower than the whole thing of locating LDT?

Try measuring that. I think that LAR will be faster than lookup in
memory, but...
>
> Let me know if I did something stupid.
> The patch is attached.
> I tested (pretty much) everything in it, except
> probably the "popl %esp" restartability. But that
> one looks fairly simple and should work.

Did you tried setting SS/ESP on stack to invalid values, so IRET on CPL1
triggers fault? You should be able to do that by modifying SS/ESP from
signal handler.

> +#define NMI_STACK_RESERVE 0x400
> +#define MAKE_ESPFIX \
> + cli; \
> + subl $NMI_STACK_RESERVE, %esp; \

This is ugly. What if NMI handler uses more than 1KB?

> + .rept 5; \
> + pushl NMI_STACK_RESERVE+0x10(%esp); \
> + .endr; \
> + pushl %eax; \
> + movl %esp, %eax; \
> + pushl %ebp; \
> + GET_THREAD_INFO(%ebp); \
> + movl TI_cpu(%ebp), %ebp; \
> + shll $GDT_SIZE_SHIFT, %ebp; \
> + addl $cpu_gdt_table, %ebp; \
> + movw %ax, ((GDT_ENTRY_ESPFIX_SS << GDT_ENTRY_SHIFT) + 2)(%ebp); \
> + shrl $16, %eax; \
> + movb %al, ((GDT_ENTRY_ESPFIX_SS << GDT_ENTRY_SHIFT) + 4)(%ebp); \
> + movb %ah, ((GDT_ENTRY_ESPFIX_SS << GDT_ENTRY_SHIFT) + 7)(%ebp); \
> + popl %ebp; \
> + popl %eax; \
> + pushw 14(%esp); \

What about referencing 14+NMI_STACK_RESERVE() ? And ESP is not
multiple of 4 here - it may slow down processor a bit.

I think that you should prepare stack by using moves to
-NMI_STACK_RESERVE-xx(%esp) instead of adjusting stack and using pushes.
Or you can (if you want per-thread and not per-CPU stack) prepare CPL1
stack above CPL0 stack - this way you steal 24 bytes from CPL0 stack,
but CPL0 can use full 4KB (8KB), without NMI being limited by 1KB.

> + pushw $4; \
> + pushl $__ESPFIX_SS; \
> + pushl $0; \
> + pushl 20(%esp); \
> + andl $~(IF_MASK | TF_MASK | RF_MASK | NT_MASK | AC_MASK), (%esp); \
> + orl $IOPL1_MASK, (%esp); \

Does this work if userspace runs with iopl 3,2 or 1? Does iret on CPL1
set iopl level properly (i.e. is iopl set if IOPL <= CPL, or only if CPL=0?).

> + pushl $__ESPFIX_CS; \
> + pushl $espfix_trampoline;
> +
> #define SAVE_ALL \
> cld; \
> pushl %es; \
> @@ -122,9 +172,24 @@
> .previous
>
>
> +/* If returning to Ring-3, not to V86, and with
> + * the small stack, try to fix the higher word of
> + * ESP, as the CPU won't restore it from stack.
> + * This is an "official" bug of all the x86-compatible
> + * CPUs, which we can try to work around to make
> + * dosemu happy. */
> #define RESTORE_ALL \
> - RESTORE_REGS \
> - addl $4, %esp; \
> + movl EFLAGS(%esp), %eax; \
> + movb CS(%esp), %al; \
> + andl $(VM_MASK | 2), %eax; \
> + cmpl $2, %eax; \
> + jne 3f; \
> + larl OLDSS(%esp), %eax; \
> + testl $0x00400000, %eax; \
> +3: RESTORE_REGS \
> + leal 4(%esp), %esp; \
> + jnz 1f; \
> + MAKE_ESPFIX \
> 1: iret; \
> .section .fixup,"ax"; \

I'm not sure about lea speed. And fast path should NOT jump,
forward jumps are initialy predicted as not taken... Maybe:

3: RESTORE_REGS
jz 8f
add $4,%esp
1: iret
8: add $4,%esp
MAKE_ESPFIX
iret

And then you can move MAKE_ESPFIX out of macro.

And it seems to me that it adds too many operations to fast path.
Maybe you want to introduce some per-thread flag, or leave syscall path
to corrupt ESP (dosemu apps should not call Linux INT syscall, yes?).

> diff -urN linux-2.6.8-pcsp/arch/i386/kernel/process.c linux-2.6.8-stacks/arch/i386/kernel/process.c
> --- linux-2.6.8-pcsp/arch/i386/kernel/process.c 2004-06-21 09:19:07.000000000 +0400
> +++ linux-2.6.8-stacks/arch/i386/kernel/process.c 2004-09-21 10:45:55.000000000 +0400
> @@ -225,7 +225,7 @@
> printk("EIP: %04x:[<%08lx>] CPU: %d\n",0xffff & regs->xcs,regs->eip, smp_processor_id());
> print_symbol("EIP is at %s\n", regs->eip);
>
> - if (regs->xcs & 3)
> + if (regs->xcs & 2)
> printk(" ESP: %04x:%08lx",0xffff & regs->xss,regs->esp);

I think you should leave this as is, regs->xss/regs->esp should be valid
for CPL1 exceptions.

> printk(" EFLAGS: %08lx %s (%s)\n",regs->eflags, print_tainted(),UTS_RELEASE);
> printk("EAX: %08lx EBX: %08lx ECX: %08lx EDX: %08lx\n",
> diff -urN linux-2.6.8-pcsp/arch/i386/kernel/signal.c linux-2.6.8-stacks/arch/i386/kernel/signal.c
> --- linux-2.6.8-pcsp/arch/i386/kernel/signal.c 2004-08-10 11:02:36.000000000 +0400
> +++ linux-2.6.8-stacks/arch/i386/kernel/signal.c 2004-09-21 10:48:38.000000000 +0400
> @@ -558,7 +558,7 @@
> * kernel mode. Just return without doing anything
> * if so.
> */
> - if ((regs->xcs & 3) != 3)
> + if (!(regs->xcs & 2))
> return 1;

Really? I think that it should stay.

> @@ -630,7 +630,7 @@
> /* If this is a kernel mode trap, save the user PC on entry to
> * the kernel, that's what the debugger can make sense of.
> */
> - info.si_addr = ((regs->xcs & 3) == 0) ? (void *)tsk->thread.eip :
> + info.si_addr = ((regs->xcs & 2) == 0) ? (void *)tsk->thread.eip :
> (void *)regs->eip;

Sure? Maybe this should be (regs->xcs & 3 != 3) instead?

Rest looks fine, but I'm not completely sure that SMP and NMIs are
handled in the best possible way.
Petr Vandrovec
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 14:06    [W:0.268 / U:0.732 seconds]
©2003-2014 Jasper Spaans. Advertise on this site