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

[Keir Fraser <Keir.Fraser@xxxxxxxxxxxx>]

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

We could improve the flipping code by detecting accesses that overlap
Xen's private area and simply propagate the GPF in that case. That
would stop us flipping to a zero-length segment. 

Modifying that code again is not high on our priority list,
though. Our main aim is compatibility with non-buggy programs. If
there is a minuscule chance of buggy programs looping instead of
seg-faulting, we can accept that for now. :-)

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

Well, maybe the path thru the segment-flip code is a few hundred
instructions. I bet there are plenty of syscall paths of *at least*
that length that don't contain a yield point. The major cost of the
flipping is actually the privilege-level changes (ring 3 to 0 to 3).
An application can trivially cause lots fo those, even with no
seg-flipping. 

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

I agree it is not the best situation. But to fix it we would need a
much more complex decoder, and a comprehensive test suite
(implementing the decoder without testing every instruction it can
decode would just lead to latent bugs in Xen that could be worse than
having no decoding ability for those instructions at all). It's just
more effort than we want to commit to -- the main purpose of the
seg-flipping is to allow existing distros to initially boot and allow
the sysadmin to move /lib/tls out of the way.

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

Your analysis of these opcodes (A1 and A3) is correct, but both have
code 'O|4' in the insn_decode table, so they don't go thru 'case 1:'. 

A0 and A2 have code 'O|1' which are instructions:
 MOV moffs8,AL   ;   MOV AL,moffs8
These have a single-byte offset incoded within the instruction.
The 'case 1:' *is* needed for A0 and A2.

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

Yes, this needs fixing.

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

Yes, this also needs fixing. :-)

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