2009-06-16 21:23:17

by Stas Sergeev

[permalink] [raw]
Subject: Re: Ping: Re: [PATCH 0/2] i386: espfix fixes

Hi.

17.06.2009 00:03, Alexander van Heukelum wrote:
> Just wanted to get a little attention for the espfix fixes...
>
> http://lkml.org/lkml/2009/6/7/106 : fix return to 16-bit stack from NMI
> http://lkml.org/lkml/2009/6/7/109 : fix/simplify espfix, move it into
> assembly
>
> Any objections against those?
Well, the dynamic limits were advocated
by Zach and Chuck, so lets ask them first
(added CCs).

For the rest:
Acked-by: Stas Sergeev <[email protected]>

In particular, I am asking now myself why
have I used THREAD_SIZE there, and can't
recall the reason...


2009-06-16 22:41:48

by Alexander van Heukelum

[permalink] [raw]
Subject: Re: Ping: Re: [PATCH 0/2] i386: espfix fixes

On Wed, 17 Jun 2009 01:19:05 +0400, "Stas Sergeev" <[email protected]> said:
> Hi.
>
> 17.06.2009 00:03, Alexander van Heukelum wrote:
> > Just wanted to get a little attention for the espfix fixes...
> >
> > http://lkml.org/lkml/2009/6/7/106 : fix return to 16-bit stack from NMI
> > http://lkml.org/lkml/2009/6/7/109 : fix/simplify espfix, move it into
> > assembly
> >
> > Any objections against those?
> Well, the dynamic limits were advocated
> by Zach and Chuck, so lets ask them first
> (added CCs).

Hi!

I see...

On http://lkml.indiana.edu/hypermail/linux/kernel/0608.0/0162.html:
> > > - .quad 0x0000920000000000 /* 0xd0 - ESPFIX 16-bit SS */
> > > + .quad 0x00cf92000000ffff /* 0xd0 - ESPFIX SS */
> >
> > Seems a bit dangerous to allow access to full 4GB through this.
> > Can you tighten the limit any? I suppose not, because the high
> > bits in %esp really could be anything. But it might be nice to
> > try setting the limit to regs->esp + THREAD_SIZE. Of course, this
> > is not strictly necessary, just an extra paranoid protection
> > mechanism.
> Since, when calculating the base, I do &-THREAD_SIZE, I guess the
> minimal safe limit is regs->esp + THREAD_SIZE*2... Well, may just
> I not do that please? :) For what, btw? There are no such a things
> for __KERNEL_DS or anything, so I just don't see the necessity.

In the normal case user-esp would be between 0 and 65535, and in
that case the memory range in the ESPFIX stack segment would be
pretty small. But userspace can set esp to just about anything if
it really wants to, and in that case the reduction of the memory
range is pretty much wothless (kernel stacks are allocated way
above the code). As you said: may we just not do that, please?

> For the rest:
> Acked-by: Stas Sergeev <[email protected]>

Thanks, very much appreciated!

> In particular, I am asking now myself why
> have I used THREAD_SIZE there, and can't
> recall the reason...

Other parts of the complete fix were not massochistic enough?
( http://lkml.indiana.edu/hypermail/linux/kernel/0608.0/0858.html )

Greetings,
Alexander

2009-06-16 23:34:25

by H. Peter Anvin

[permalink] [raw]
Subject: Re: Ping: Re: [PATCH 0/2] i386: espfix fixes

Alexander van Heukelum wrote:
>
> On http://lkml.indiana.edu/hypermail/linux/kernel/0608.0/0162.html:
>>>> - .quad 0x0000920000000000 /* 0xd0 - ESPFIX 16-bit SS */
>>>> + .quad 0x00cf92000000ffff /* 0xd0 - ESPFIX SS */
>>> Seems a bit dangerous to allow access to full 4GB through this.
>>> Can you tighten the limit any? I suppose not, because the high
>>> bits in %esp really could be anything. But it might be nice to
>>> try setting the limit to regs->esp + THREAD_SIZE. Of course, this
>>> is not strictly necessary, just an extra paranoid protection
>>> mechanism.
>> Since, when calculating the base, I do &-THREAD_SIZE, I guess the
>> minimal safe limit is regs->esp + THREAD_SIZE*2... Well, may just
>> I not do that please? :) For what, btw? There are no such a things
>> for __KERNEL_DS or anything, so I just don't see the necessity.
>
> In the normal case user-esp would be between 0 and 65535, and in
> that case the memory range in the ESPFIX stack segment would be
> pretty small. But userspace can set esp to just about anything if
> it really wants to, and in that case the reduction of the memory
> range is pretty much wothless (kernel stacks are allocated way
> above the code). As you said: may we just not do that, please?
>

Who are "we", here? First of all, this is about a 16-bit stack segment,
right? Why are we doing a 4 GB limit at all if this is a 16-bit stack
segment (where the stack can only be touched for the first 64K even if a
stack operation happens)?

I clearly don't quite understand at a glance what is going on.

-hpa