[PATCH 0/2+0/2] GUI daemon/shmoverride support for multiple X servers

[Ilpo Järvinen]

Hi all,

This patchset enables shmoverride/qubes-guid daemon support
for running multiple X servers concurrently. It allows
having one (or some) appvm GUIs under X server(s) that
runs under other virtual console. The need for this has
come up at mailing list from time to time, e.g., opening
windows VM in a second virtual console [1] or an ability to
connect appvm directly to login screen by having a custom
xsession for that [2]. The custom xsession should also allow
limited multi-user support (however, Qubes RPC support for
that kind of usecase is currently very limited/does not
exits for more than one X server but denying cross-user
RPCs manually might be enough for start).

If anyone has any thought / directions on how / what
should be implement for better Qubes RPC support for
the multi-user/X server case, please share.

This patchset is tested in R3.2. There are two patches for
gui-daemon (the first is just to introduce a variable for
shm.id filename and the second is for the actual logic changes).
The gui-daemon changes in this patchset depends on 3c86bef963
(Move /var/run/shm.id to /var/run/qubes/shm.id) which
is not in R3.2 (I cherrypicked it to R3.2 to make master
integration easier). The DISPLAY value is extracted from
/proc/self/cmdline for the shmoverride constructor which
I hope is an acceptable solution. I'm unsure what the code
should do if there is no DISPLAY found on the cmdline,
perhaps it should just bail out instead of returning :0
as it does in the current changeset?

There are two patches for core-admin. The second patch
for core-admin is not strictly necessary for R3.2 but
in order to make it easier to turn it master compatible
I added the second cludge change for testing in a more
master like codebase that supports migration from one
shm.id path to another. If requested, I can provide the
master based patch with the end result, however, testing
with master/R4.0 does not seem realistic for me at the
moment (is that required?).


--
i.


[1] https://groups.google.com/forum/#!topic/qubes-devel/6inz6v2ByNo
[2] https://groups.google.com/forum/#!msg/qubes-devel/XnTly2JNfPw/BppN_D5vn4kJ

[Marek Marczykowski-Górecki]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On Fri, Dec 23, 2016 at 12:21:54AM +0200, Ilpo Järvinen wrote:
> Hi all,
>
> This patchset enables shmoverride/qubes-guid daemon support
> for running multiple X servers concurrently. It allows
> having one (or some) appvm GUIs under X server(s) that
> runs under other virtual console. The need for this has
> come up at mailing list from time to time, e.g., opening
> windows VM in a second virtual console [1] or an ability to
> connect appvm directly to login screen by having a custom
> xsession for that [2]. The custom xsession should also allow
> limited multi-user support (however, Qubes RPC support for
> that kind of usecase is currently very limited/does not
> exits for more than one X server but denying cross-user
> RPCs manually might be enough for start).
>
> If anyone has any thought / directions on how / what
> should be implement for better Qubes RPC support for
> the multi-user/X server case, please share.

This alone is far from multi-user system. If you have access to dom0,
you can execute any command in any VM. Even if you wouldn't see GUI of
that VM. This isn't only about qrexec. For example you need access to
libvirt daemon to start a VM and when you have it, you can access any VM
configuration, console etc. There are a lot more things like this.

The proper solution for this is GUI domain, where local user
will not have direct access to dom0 and only interact through set of
qrexec services, subject to qrexec policy. Here is a WIP
architecture:
https://www.qubes-os.org/doc/mgmt-architecture/

But multi-X server environment still may be helpful in some cases, as
you've noted above.

> This patchset is tested in R3.2. There are two patches for
> gui-daemon (the first is just to introduce a variable for
> shm.id filename and the second is for the actual logic changes).

I think the first patch alone (without the second one) will result in
not compiling code. Because of this:
+#define SHMID_FILENAME_LEN (sizeof(SHMID_FILENAME_PREFIX))

And SHMID_FILENAME_PREFIX is defined only in the second patch...

> The gui-daemon changes in this patchset depends on 3c86bef963
> (Move /var/run/shm.id to /var/run/qubes/shm.id) which
> is not in R3.2 (I cherrypicked it to R3.2 to make master
> integration easier). The DISPLAY value is extracted from
> /proc/self/cmdline for the shmoverride constructor which
> I hope is an acceptable solution. I'm unsure what the code
> should do if there is no DISPLAY found on the cmdline,
> perhaps it should just bail out instead of returning :0
> as it does in the current changeset?

I think the X server behaviour is to use ":0". For example if you try to
start second X instance without specifying display, it will fail.
So, I'm fine with this approach.

Actually, lack of a method for extracting DISPLAY in shmoverride init
call was the only reason why it wasn't implemented earlier. The idea of
/proc/self/cmdline is ok.

> There are two patches for core-admin. The second patch
> for core-admin is not strictly necessary for R3.2 but
> in order to make it easier to turn it master compatible
> I added the second cludge change for testing in a more
> master like codebase that supports migration from one
> shm.id path to another. If requested, I can provide the
> master based patch with the end result, however, testing
> with master/R4.0 does not seem realistic for me at the
> moment (is that required?).

Change like this shouldn't go into stable release, as explained in github
issue[1]. Unless you convince me that every case (like upgrading one
package but not the other) is covered...
But don't worry - the tricky part (gui-daemon) is the same in
R3.2 and R4.0, so testing it on R3.2 is ok. core-admin code is very
different, but the patch is easy to apply manually there. And actually
I'd reduce code duplication there - to have shm.id path constructed in
one place, not three.

And one more thing: please sign your code. Details:
https://www.qubes-os.org/doc/code-signing/

[1] https://github.com/QubesOS/qubes-issues/issues/2195
- --
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJYXKSdAAoJENuP0xzK19csCX0H/jEaRswMFvBddc0qhbUHkRTY
K4KiXCKlOvUPh20ScQK0493NHfwaPfxdB5lLrXNV3r/N9KTcDxGFjhjptfpHUWAK
p9rWYgBDyO3bT4sZd91Fwufy5c4bnaaw0ablHIFjsMBJsk6TICRcGDql2qZXR6YQ
pFs1PEvA+yD2npbK+CBj8IVxFpdWxosIcx5oMgWrkLLQTbHD2L8tdhJ6VufIBhT/
6ljyc4ztF1B2mJlSa1ABIInh2ljZs59vCNVJAJZmIkRqKZenqLpDAwa+nXLSmJ6D
788z8IiZMJDBo6fvaiszEmm0P6a8eTjvIaDcE05c0noa8CHA9Y9vRGh1cjcFWMY=
=eM+M
-----END PGP SIGNATURE-----

[Ilpo Järvinen]

All true. But this wasn't exactly what I was thinking though (I know
very well that dom0 access cannot be allowed)... What I meant was,
e.g., copy-paste (others too?) that need to be instanciated for each
user rather than having a global clipboard.

With "limited multi-user support" I meant something along these lines:
A special "launchvm" that is the only thing that an ordinary (non-dom0)
user gets when logging in. The sole purpose of the launchvm is to
qvm-run the real applications in the actual user appvms (policy set to
allow this from launchvm for the user own appvms). That should probably
suffice for an initial version in the shared family computer use case
I mentioned earlier. Limited, but better than non-Qubes solutions for
sure.

However, the "launchvm" concept is of course already quite close
to what management API aims to cover (especially if "launchvm"
extented beyond qvm-running).

> The proper solution for this is GUI domain, where local user
> will not have direct access to dom0 and only interact through set of
> qrexec services, subject to qrexec policy. Here is a WIP
> architecture:
> https://www.qubes-os.org/doc/mgmt-architecture/

It sounds very interesting.

Maybe this is a stupid question: Is managing the physical machine
itself out-of-scope for this API (mainly to support shutdown/reboot
I guess)?

> But multi-X server environment still may be helpful in some cases, as
> you've noted above.
>
> > This patchset is tested in R3.2. There are two patches for
> > gui-daemon (the first is just to introduce a variable for
> > shm.id filename and the second is for the actual logic changes).
>
> I think the first patch alone (without the second one) will result in
> not compiling code. Because of this:
> +#define SHMID_FILENAME_LEN (sizeof(SHMID_FILENAME_PREFIX))
>
> And SHMID_FILENAME_PREFIX is defined only in the second patch...

Ah, yes, I'm sorry about that. Will be corrected in the next
version (and I try to remember at least compile test all
intermediate steps too in future patches. For some reason
entirely forgot it this time after I made some simplification
to the original version of the first patch).

> > There are two patches for core-admin. The second patch
> > for core-admin is not strictly necessary for R3.2 but
> > in order to make it easier to turn it master compatible
> > I added the second cludge change for testing in a more
> > master like codebase that supports migration from one
> > shm.id path to another. If requested, I can provide the
> > master based patch with the end result, however, testing
> > with master/R4.0 does not seem realistic for me at the
> > moment (is that required?).
>
> Change like this shouldn't go into stable release, as explained in github
> issue[1]. Unless you convince me that every case (like upgrading one
> package but not the other) is covered...

Of course not, I wasn't expecting it to go to R3.2. I mentioned
all this just to indicate how I tested it and for the benefit
of those people who might want to test/use the patches themselves
with the current stable release.

> But don't worry - the tricky part (gui-daemon) is the same in
> R3.2 and R4.0, so testing it on R3.2 is ok. core-admin code is very
> different, but the patch is easy to apply manually there. And actually
> I'd reduce code duplication there - to have shm.id path constructed in
> one place, not three.

Indeed, that would be much cleaner approach. I'll add that change to
the second version of the changeset.

> And one more thing: please sign your code. Details:
> https://www.qubes-os.org/doc/code-signing/

Ok, I'll try (no prior experience gpg usage with email).


Thanks for the review and suggestions.


--
i.

[Marek Marczykowski-Górecki]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

So, user interact only with "launchvm", right? How you envision to
achieve this? In GUI domain concept it is achieved by attaching
input/output devices to GUI domain instead of dom0.

> The sole purpose of the launchvm is to
> qvm-run the real applications in the actual user appvms (policy set to
> allow this from launchvm for the user own appvms). That should probably
> suffice for an initial version in the shared family computer use case
> I mentioned earlier. Limited, but better than non-Qubes solutions for
> sure.
>
> However, the "launchvm" concept is of course already quite close
> to what management API aims to cover (especially if "launchvm"
> extented beyond qvm-running).

Yes, exactly.

> > The proper solution for this is GUI domain, where local user
> > will not have direct access to dom0 and only interact through set of
> > qrexec services, subject to qrexec policy. Here is a WIP
> > architecture:
> > https://www.qubes-os.org/doc/mgmt-architecture/
>
> It sounds very interesting.
>
> Maybe this is a stupid question: Is managing the physical machine
> itself out-of-scope for this API (mainly to support shutdown/reboot
> I guess)?

You may be allowed to perform actions on dom0, like mgmt.vm.Shutdown. In
extreme case, even qubes.VMShell - but then GUI domain isolation is
somehow less meaningful.
But on the other hand, when GUI will be moved to separate domain, there
will be much less things to needed to be configured there. Those few
things left will probably get some additional qrexec services for
managing them.

> > But don't worry - the tricky part (gui-daemon) is the same in
> > R3.2 and R4.0, so testing it on R3.2 is ok. core-admin code is very
> > different, but the patch is easy to apply manually there. And actually
> > I'd reduce code duplication there - to have shm.id path constructed in
> > one place, not three.
>
> Indeed, that would be much cleaner approach. I'll add that change to
> the second version of the changeset.
>
> > And one more thing: please sign your code. Details:
> > https://www.qubes-os.org/doc/code-signing/
>
> Ok, I'll try (no prior experience gpg usage with email).

If you like, you can also push git repository somewhere (github or
else), with signed commits and/or signed tag at the top.

- --
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJYXb0BAAoJENuP0xzK19cskPoH/1kqzEkYVImiyQHcpAdHsuA3
vYQT5W+1KCQRxtUSCD5R7JfyrnbVqENiSgYgn9SSze302ky9ojs6qlJO7NaX1ror
skp/zaxALG93Wn8o8txY06so22neeY5cYtW5K+C8Z8kw4qBunQCE+6fhxgcaiwM+
nO/w/zOxwAyLBk/VJtUi3XwvudLa1TNnekMCRS84vDyr14OfjZQHifaJdN+U7tJd
DsYFlG4f6I//eq6dYPF4/puZ8S7YYgGOsDG0nHJf3lhG0xZn2tujTrcpU3yOijJ3
Vtw0d7kG01XTqQLJgbF+Uh3IgJ0tcOxsYDnK7ZMuChao3zkWy3klzaqO3QGj9tg=
=u+xK
-----END PGP SIGNATURE-----

[Ilpo Järvinen]

On Sat, 24 Dec 2016, Marek Marczykowski-Górecki wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
>
> On Sat, Dec 24, 2016 at 12:12:10AM +0200, Ilpo Järvinen wrote:
> > On Fri, 23 Dec 2016, Marek Marczykowski-Górecki wrote:
> >
> > > -----BEGIN PGP SIGNED MESSAGE-----
> > > Hash: SHA256
> > >
> > > On Fri, Dec 23, 2016 at 12:21:54AM +0200, Ilpo Järvinen wrote:
> >
> > With "limited multi-user support" I meant something along these lines:
> > A special "launchvm" that is the only thing that an ordinary (non-dom0)
> > user gets when logging in.
>
> So, user interact only with "launchvm", right? How you envision to
> achieve this? In GUI domain concept it is achieved by attaching
> input/output devices to GUI domain instead of dom0.

I/O would be done "like now". The user interacts through the X server in
dom0 but the user input on that X server should be limited to mainly
managing focus and screenlocking. In my threat model, the users are not
exactly a threat other than accidentially messing something if the
system is not enough locked down for them.

The key difference between "launchvm" and mgmt+GUI VM is just where
the X servers run. Cross-user separation aspects are the same
regardless where GUI is done as the GPU PCI device cannot be
shared between many VMs anyway (except perhaps with XenGT one day)
so X servers for all users in multi-user case need to run in the
same VM. Obviously dom0 itself is better isolated in the GUI domain
case so less tricks may be needed but I don't think GUI domain itself
is a magic bullet that solves all multi-user related problems.

> > > And one more thing: please sign your code. Details:
> > > https://www.qubes-os.org/doc/code-signing/
> >
> > Ok, I'll try (no prior experience gpg usage with email).
>
> If you like, you can also push git repository somewhere (github or
> else), with signed commits and/or signed tag at the top.

Ok, I will probably then end up using http git repo as I don't want
to create yet-another-account github would require (at least for now).

Anyway, it will take week or slightly more before I can test the
next version as I intentionally left myself without access to my
qubes laptop for a while :-D.


--
i.

[Ilpo Järvinen]

And of course, I'm not saying that things must remain in dom0...
I see it just a necessary evil if there's no better solution
available.

Hmm, an additional thought: Why not moving as much as possible
away from the gui domain into "a userctrl VM" also in the gui
domain/mgmt API case? I could try to do a drawing about that idea
once I find some time (if I find the source for that figure on
the mgmt page).


--
i.

[Jean-Philippe Ouellet]

On Sat, Dec 24, 2016 at 4:55 AM, Ilpo Järvinen
<ilpo.j...@helsinki.fi> wrote:
> I could try to do a drawing about that idea once I find some time
> (if I find the source for that figure on the mgmt page).

Source is here [1], created with Dia [2].

Also, I have not reviewed the implementation of your patch, but I
agree with the concept.

[1]: https://github.com/QubesOS/qubes-attachment/tree/master/wiki/mgmt
[2]: https://en.wikipedia.org/wiki/Dia_(software)

[Marek Marczykowski-Górecki]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On Sat, Dec 24, 2016 at 10:56:41AM +0200, Ilpo Järvinen wrote:
> On Sat, 24 Dec 2016, Marek Marczykowski-Górecki wrote:
>
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA256
> >
> > On Sat, Dec 24, 2016 at 12:12:10AM +0200, Ilpo Järvinen wrote:
> > > On Fri, 23 Dec 2016, Marek Marczykowski-Górecki wrote:
> > >
> > > > -----BEGIN PGP SIGNED MESSAGE-----
> > > > Hash: SHA256
> > > >
> > > > On Fri, Dec 23, 2016 at 12:21:54AM +0200, Ilpo Järvinen wrote:
> > >
> > > With "limited multi-user support" I meant something along these lines:
> > > A special "launchvm" that is the only thing that an ordinary (non-dom0)
> > > user gets when logging in.
> >
> > So, user interact only with "launchvm", right? How you envision to
> > achieve this? In GUI domain concept it is achieved by attaching
> > input/output devices to GUI domain instead of dom0.
>
> I/O would be done "like now". The user interacts through the X server in
> dom0 but the user input on that X server should be limited to mainly
> managing focus and screenlocking. In my threat model, the users are not
> exactly a threat other than accidentially messing something if the
> system is not enough locked down for them.

How would you define multi-user system? I think isolating one user
(data) from another is fair requirement for it. So, here the user is a
threat in some sense.

> The key difference between "launchvm" and mgmt+GUI VM is just where
> the X servers run. Cross-user separation aspects are the same
> regardless where GUI is done as the GPU PCI device cannot be
> shared between many VMs anyway (except perhaps with XenGT one day)
> so X servers for all users in multi-user case need to run in the
> same VM.

If using GUI domain for multi-user system, then each user would have own
GUI domain. Only one of them would be running (or have access to GPU) at
the same time.

Anyway, I think multi-user system, is not so needed feature.

> Obviously dom0 itself is better isolated in the GUI domain
> case so less tricks may be needed but I don't think GUI domain itself
> is a magic bullet that solves all multi-user related problems.

Yes, certainly it will not be enough itself. But will help a lot.

> > > > And one more thing: please sign your code. Details:
> > > > https://www.qubes-os.org/doc/code-signing/
> > >
> > > Ok, I'll try (no prior experience gpg usage with email).
> >
> > If you like, you can also push git repository somewhere (github or
> > else), with signed commits and/or signed tag at the top.
>
> Ok, I will probably then end up using http git repo as I don't want
> to create yet-another-account github would require (at least for now).
>
> Anyway, it will take week or slightly more before I can test the
> next version as I intentionally left myself without access to my
> qubes laptop for a while :-D.

:)

- --
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJYXujlAAoJENuP0xzK19cs/5QIAJMUxoy7NGbsYLe4TwYdVHU8
gu6smEhz+EbVdyYwC91K9mFVpLPomQcUJaLzM/5AL/f5hhxUBfUZvvc5WruYQst9
gD6CznshUg/zI5ryqoqrKOESAA6ceYuaL330U0ujP8tpgpvK6zFfLu+vgPpvfxw+
CErz1wMgSfYptQU1qrZTvkZnGtdKSb2tVIbkQea6AzTFBUpr9jW4W/uQsGne9EQO
KmQFZ+p9EFApnpwVN6NbgX3kyfCxU7zjZsLiSHaImT3YWL9RsJH7lzVX6DN9+pw3
th5ril7YFCiLoho+d+4pgtZ618aua0L+2UnJ1OB4F8KZpneGS5UeDsNLkQgTbDw=
=4gen
-----END PGP SIGNATURE-----

[Ilpo Järvinen]

On Sat, 24 Dec 2016, Marek Marczykowski-Górecki wrote:

> > > But don't worry - the tricky part (gui-daemon) is the same in
> > > R3.2 and R4.0, so testing it on R3.2 is ok. core-admin code is very
> > > different, but the patch is easy to apply manually there. And actually
> > > I'd reduce code duplication there - to have shm.id path constructed in
> > > one place, not three.
> >
> > Indeed, that would be much cleaner approach. I'll add that change to
> > the second version of the changeset.
> >
> > > And one more thing: please sign your code. Details:
> > > https://www.qubes-os.org/doc/code-signing/
> >
> > Ok, I'll try (no prior experience gpg usage with email).
>
> If you like, you can also push git repository somewhere (github or
> else), with signed commits and/or signed tag at the top.

The updated changes are now in git repos (hopefully I got
signed commits to work properly):

https://www.cs.helsinki.fi/u/ijjarvin/qubes/gui-daemon/
https://www.cs.helsinki.fi/u/ijjarvin/qubes/core-admin/

There are two branches in both repositories: release3.2 is what
I used for testing in R3.2 and a master compatible one. I did
slight improvements to the cmdline parser (and tested it with
various valid and invalid inputs using external wrapper tool).

As discussed earlier, the release3.2 is not intented to be
included to the stable version but just to indicate what was
actually tested and for the benefit of those who would might
want to use them themselves already in R3.2.

In the master branch the changes are untested but for gui-daemon
it was just a simple cherrypick and after handling first the code
duplication like you suggested, also core-admin changes became
much easier to compare between release3.2 and master branches.
Thus, I hopefully got the master versions right too so you can
simply pull them (rather than resort to handcrafted modifications).


--
i.

[Marek Marczykowski-Górecki]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On Sun, Jan 08, 2017 at 04:07:01PM +0200, Ilpo Järvinen wrote:
> The updated changes are now in git repos (hopefully I got
> signed commits to work properly):
>
> https://www.cs.helsinki.fi/u/ijjarvin/qubes/gui-daemon/
> https://www.cs.helsinki.fi/u/ijjarvin/qubes/core-admin/
>
> There are two branches in both repositories: release3.2 is what
> I used for testing in R3.2 and a master compatible one. I did
> slight improvements to the cmdline parser (and tested it with
> various valid and invalid inputs using external wrapper tool).
>
> As discussed earlier, the release3.2 is not intented to be
> included to the stable version but just to indicate what was
> actually tested and for the benefit of those who would might
> want to use them themselves already in R3.2.
>
> In the master branch the changes are untested but for gui-daemon
> it was just a simple cherrypick and after handling first the code
> duplication like you suggested, also core-admin changes became
> much easier to compare between release3.2 and master branches.
> Thus, I hopefully got the master versions right too so you can
> simply pull them (rather than resort to handcrafted modifications).

Hi,

I've merged it already - master branch in gui-daemon, core3-misc-fixes2
branch in my core-admin (will land in core3-devel soon and then master).
Thanks!

There was one issues: X server get ":0" display, but $DISPLAY is set to
":0.0", so if the string is used directly it does not work. I've changed
it to just display number ("0"), instead of the whole string.

- --
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJYkHU8AAoJENuP0xzK19csNa8H/2ZzVlLjyWJoSJ4ht++ZaOen
A68vVAScEZ2ubXMYrpFZI21SEZYaWPI4Zu4Qtl/AwN5wMR9b2sevxdWlvTEl5Lm2
2Ra85lPXk/RoPMn0uHJhqhLmSar9VwsEsGmS1mVI14WPU0AjomM7aAymnlnmPtx9
4k9B2/eACAU9TyfYr1eBMTmZOUgRv8vqht9CA5CS92aNRGfSerY3uPsmX3hawzcA
ovp6WEqvDybUQeGu8lsE56NNU9Se8gP1E6qxzm5HJFhE78nUE6NxggnxvO7kWs30
ZMd2oAWutJ9Jrgduz5sLdXRaMP26296glwpFe2VyBcf/iTKOz/0btGKH/AewK3c=
=POQI
-----END PGP SIGNATURE-----