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

["Jan Beulich" <JBeulich@xxxxxxxxxx>]

>> 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.]

Oh, right, I didn't connect the -1 done after the flip: label. But it
seems pointless to flip a segment with a base of zero; the gp fault must
have had a different reason (i.e. by flipping it to a zero-length
segment you'll only cause another gp fault right away when restarting
the instruction).

>> (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.

Not exactly: if a user mode process runs en infinite loop, it'll not
block out interrupts for any more than a single instruction. For the
mentioned scenario, (physical) interrupt latency would significantly
increase.

>> (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.

Except that the DPRINTK expands to nothing in the published sources, so
one would never know what caused a spurious SEGV seen in a client OSes
app without re-running the whole thing with a version of XEN where these
DPRINTKs are actually doing something.

>> (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;

Yes.

>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.

I don't think so. The instructions this refers to are (opcodes A1 and
A3)

        movb    symbol, %al
        movb    %al, symbol

Clearly, they take a full (16-/32-bit) address but operate on a byte at
that address (they would be useless if they operated only on an 8-bit
sign-extended address).
Having a second look at this reveals another dangerous thing: The code
here directly derefences pb, whereas all other instances of references
through ip correctly use get_user.
Finally, in the case I'm still missing something here and the 'case 1'
is needed, then an operation like (long)*(char *) is rather dangerous,
because you depend on the (implementation defined) signedness of char.
You'd want to code (long)*(signed char *) instead.

Jan


-------------------------------------------------------
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