[vbox-dev] Chromium WindowDestroy

[Chris Smowton]

On 17/10/09 16:00, Leonid wrote:
>> * Has anyone hit this particular issue before? If so, what did they do?
>> * Does my plan above sound sane? Any suggestions?
>> * What's the proper way to set the system context (i.e. really run
>> glxMakeCurrent) without introducing inconsistencies in server state?
> ... [the] render spu should bind default window to all contexts if its 
> currently
> bound window is being destroyed. I've commited the fix, so it should 
> be in the
> ose soon. Could you please check if your issue is gone and post a 
> followup here.
I just finished coding a similar solution, I'll see how our changes 
compare :)

In the course of doing so, I got to know the render SPU's "default 
window," which it creates at startup and uses to perform real-libGL 
MakeCurrents when the client hasn't sent us a WindowCreate yet, and the 
client might send GL commands which are legal without a current context 
(e.g. create/destroy-context) which on the server are executed using GL 
commands which *do* require a current context -- like the previous 
example with state_diff using glEnable and other state functions.

This looks like a minor problem -- a client could skip issuing a 
WindowCreate and start sending rendering commands, and the render SPU 
would map the default window. The default window is not a child of the 
VBox guest display window, so there would just be a random top-level 
window containing the guest's 3D content. It seems to me the guest 
shouldn't be able to "escape" its bounds like this, even cosmetically -- 
and further, it means Chromium is accepting what would be an illegal 
OpenGL program -- moving straight to glBegin(); glVertex... without a 
glX/wgl/agl/cglCreateContext+MakeCurrent ought to result in an error.

A separate issue I found is that in the server's implementation of 
WindowDestroy, it notes that "some apps destroy windows in a different 
thread than they created it", and so spools through all the current 
clients, killing like-numbered windows. It seems to me that there ought 
to be a line between threads in one process and separate processes, as 
otherwise we could have the following situation:

A guest could run a regular GL program (say, gears,) and a malicious 
program. The malicious program could simply guess at GL's CrWindow ID 
and send a WindowDestroy; the server would then regard that as an 
out-of-thread destroy, and zap gears' window, which would then try to 
draw illegally -- and in fact, hit the default zero-window, according to 
your new fix. This seems to me as though it's allowing processes within 
one VM to violate the isolation which their operating system is supposed 
to provide. The same ID-guessing game could be played with contexts, and 
possibly other resources like textures (I haven't investigated how 
they're named by Cr yet).

I think a solution might be:

* Multithreading GL programs are spotted by the guest library, and their 
commands muxed onto a single TCP / HGCM channel. (Looks like VBox is 
using HGCM by default and TCP with a couple of undefs; I've hacked it 
back to TCP for my purposes)
* * Obviously that would mean the server needs a concept of per-thread 
concept, which it almost has already.
* Window, context and other vulnerable IDs are allocated per-connection, 
not per-VM as at present.
* Therefore, it's not possible to make reference to out-of-process 
resources.

The only problem here would be coordinating the 
new-thread-starts-rendering process, but there's a bunch of code 
detecting this situation in the Chromium libGL stub already -- the 
server would just need minor alterations to introduce the extra 
hierarchy in which it has a connection per client, which have a pool of 
contexts and windows, and the clients have threads, and the threads have 
a current-context and current-window like clients do now, with switching 
happening the same way as now. Then the server would scan for other 
threads using the same window / context -- but not other processes.

Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.virtualbox.org/pipermail/vbox-dev/attachments/20091017/30d35a20/attachment.html>