Re: [Xen-devel] NPTL/TLS segment flipping code problem

[Keir Fraser <Keir.Fraser@xxxxxxxxxxxx>]

> Looking at this code (2.0.2), it appears to have a couple of problems I
> could not find mentioned on the mailing list archive:
> 
> (1) If base is zero in an expand-up segment, the conversion will yield
> an expand-down segment covering the whole 4Gb, thus providing a
> mechanism to obtain access to XEN space.

No it won't. When we flip to expands-down we set the limit to
    (-(base & PAGE_MASK) >> 12) - 1
 == (-(0 & PAGE_MASK) >> 12) - 1
 == 0 - 1
 == 0xfffff

This is a *zero-length* grows-down segment, exactly as we require for
safety. [Yes, you can have a zero-length grows-down segment, even
though it is impossible to have a zero-length grows-up segment -- I
tested on real silicon.]

> (2) If a malicious program accesses memory at a small negative offset
> from gs:0 and the access extends into the positive range, the access
> will gp-fault with either descriptor setting, thus leading to an endless
> loop of flipping between the two states.

Harmless. The guest OS will still execute (e.g., to service timer
interrupts) and will be able to preempt the malicious program when it
has received its timeslice. So the program cannot take over the
machine -- it can only receive the same amount of CPU as a user-space
infinite CPU loop.

> (3) Since escaped opcodes (those starting with 0F) aren't handled,
> accessing mm/xmm data in __thread variables (along with other
> specialized operations on such variable the compiler might generate) is
> going to kill the program. Of course, it is similarly problematic that
> SIB addressing still isn't implemented, but that's at least stated so in
> the code.

Yes there are restrictions in what it supports. But we have a DPRINTK
for those cases so we can fix them up as/if they occur.

> (4) In the no-mod-r/m handling of the decoder, the byte case is handled
> incorrectly: The address it deals with is still a 32-byte (or 16-byte,
> but 16-bit addressing isn't handled anyway) one. There simply must not
> be a 'case 1' there, and the insn_decode table should be changed
> accordingly.

You mean the following fragment?

    if ( !(decode & HAS_MODRM) )
    {
        switch ( decode & 7 )
        {
        case 1:
            offset = (long)(*(char *)pb);
            goto skip_modrm;

It is correct -- sign extends the 8-bit offset to a signed long as we
require. The instruction only contains a single-byte offset -- it
isn't stored as 16 or 32 bits.

> (5, minor) The change from 2.0.1 to 2.0.2 (making the code a lot more
> correct) left an access to the no longer existing positive_access
> parameter of fixup_seg in (at least) one of the DPRINTK-s.

Now fixed in the new 2.0.3 release. The FC3 issue that some poeple
(including Rik) were seeing is also now fixed.

 -- Keir


-------------------------------------------------------
The SF.Net email is sponsored by: Beat the post-holiday blues
Get a FREE limited edition SourceForge.net t-shirt from ThinkGeek.
It's fun and FREE -- well, almost....http://www.thinkgeek.com/sfshirt
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/xen-devel