Re: ESP corruption bug - what CPUs are affected?

[Petr Vandrovec <vandrove@vc.cvut.cz>]

On Sat, Sep 25, 2004 at 12:36:15AM +0400, Stas Sergeev wrote:
> Hi,
> 
> Petr Vandrovec wrote:
> >In that new patch I set the const to 0xe00, which
> >is 3,5K. Is it still the limitation? I can probably
> >For 4KB stacks 2KB looks better
> OK, done that. Wondering though, for what?
> I don't need 2K myself, I need 24 bytes only.
> So what prevents me to raise the gap to 3.5K
> or somesuch? Why 2K looks better?

You run with ESP decreased by 2KB for some time during
CPL1 stack setup.  As you run in this part at CPL0
with same setup as on CPL1, I think that you should
offer same stack for setup code, and for CPL1 code,
and so each should get 2KB.

> >>+8:	subl $(NMI_STACK_RESERVE-4), %esp; \
> >>+	.rept 5; \
> >Any reason why you left this part of RESTORE_ALL macro?
> The reason is that the previous part of the macro
> can jump to that part. So how can I divide those?

Use real labels instead of numeric local labels.  That way
macro does jne cpl1exit, and you create cpl1exit: label outside
of macro, somewhere in entry.S...
 
> >be moved out, IMHO.  RESTORE_ALL is used twice in entry.S, so you
> >could save one copy.
> Do you mean the NMI return path doesn't need
> the ESP fix at all? Why?

No.  I was attempting to say that RESTORE_ALL is on two
places to save jumps (I cannot think about any other reason),
and so cpl1exit could be shared:

#define RESTORE_ALL \
	movl EFLAGS(%esp),%eax	\
	....			\
	jne 3f			\
	lar OLDSS(%esp),%eax	\
	testl $0x00400000,%eax	\
	jz cpl1exit		\
3:	RESTORE_REGS		\
	add $4,%esp		\
1:	iret			\
.section __ex_table,"a";\
	.align 4;	\
	long 1b,badss;	\
.previous


badss:	sti;		
	movl $(__USER_DS), %edx; 
	movl %edx, %ds; 
	movl %edx, %es;
	pushl $11;
	call do_exit;

cpl1exit:
	RESTORE_REGS
	subl $...,%esp
	...

But your solution with killing RESTORE_ALL completely is also
fine - you just could reorder jumps a bit, so return to kernel
or VM86 is one (not taken) jump shorter than it is with current
version of patch.

> >Though I'm not sure why NMI handler simple
> >does not jump to RESTORE_ALL we already have.
> I can only change that and then un-macro the
> RESTORE_ALL completely. So I did that.
> Additionally I introduced the "short path"
> for the case where we know for sure that we
> are returning to the kernel. And I am not
> setting the exception handler there because
> returning to the kernel if fails, should die()
> anyway. Is this correct?

If you'll first test result of lar, and then
you'll do two independent RESTORE_REGS (second
one actually does not have to restore %ebp
and %eax and you can restore them directly by
move from stack after you are done with trampoline
setup, instead of doing push + pop), you can reuse
RESOTRE_REGS+add $4,%esp+iret return path, including
its exception handling.

> +ENTRY(espfix_trampoline)
> +	popl %esp
> +espfix_past_esp:
> +1:	iret
> +.section .fixup,"ax"
> +2:	sti
> +	movl $(__USER_DS), %edx
> +	movl %edx, %ds
> +	movl %edx, %es
> +	pushl $11
> +	call do_exit
> +.previous

You can reuse fixup code from other iret instance,
just give it real label and reference it from __ex_table
instead of '2b'.

Patch looks fine, though you could speedup it a bit as outlined above.
Now you have to persuade others that this patch should include patch
into the kernel.
								Petr