7:13 p.m.
This can cause permissions failures if qemu.conf user/group is changed.
Fix this by filling in the VNC auto socket path in the same code area
that we fill in default listen address, ports, etc.
https://bugzilla.redhat.com/show_bug.cgi?id=1151762
---
src/qemu/qemu_command.c | 10 ++--------
src/qemu/qemu_process.c | 11 ++++++++++-
2 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ba15dc9..45fc63c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7544,7 +7544,6 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
static int
qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
virCommandPtr cmd,
- virDomainDefPtr def,
virQEMUCapsPtr qemuCaps,
virDomainGraphicsDefPtr graphics)
{
@@ -7561,12 +7560,7 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
goto error;
}
- if (graphics->data.vnc.socket || cfg->vncAutoUnixSocket) {
- if (!graphics->data.vnc.socket &&
- virAsprintf(&graphics->data.vnc.socket,
- "%s/%s.vnc", cfg->libDir, def->name) == -1)
- goto error;
-
+ if (graphics->data.vnc.socket) {
virBufferAsprintf(&opt, "unix:%s", graphics->data.vnc.socket);
} else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNC_COLON)) {
@@ -7944,7 +7938,7 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg,
break;
case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
- return qemuBuildGraphicsVNCCommandLine(cfg, cmd, def, qemuCaps, graphics);
+ return qemuBuildGraphicsVNCCommandLine(cfg, cmd, qemuCaps, graphics);
case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
return qemuBuildGraphicsSPICECommandLine(cfg, cmd, qemuCaps, graphics);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 605b3c6..5de46e2 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4479,7 +4479,16 @@ int qemuProcessStart(virConnectPtr conn,
goto cleanup;
}
- if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC ||
+ if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
+ !graphics->data.vnc.socket &&
+ cfg->vncAutoUnixSocket) {
+ if (virAsprintf(&graphics->data.vnc.socket,
+ "%s/%s.vnc", cfg->libDir, vm->def->name) <
0)
+ goto cleanup;
+ }
+
+ if ((graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
+ !graphics->data.vnc.socket) ||
graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
if (graphics->nListens == 0) {
if (VIR_EXPAND_N(graphics->listens, graphics->nListens, 1) < 0)
--
2.3.6
7:32 a.m.
On Tue, Apr 28, 2015 at 08:13:19PM -0400, Cole Robinson wrote:
This can cause permissions failures if qemu.conf user/group is
changed.
I assume the issue only exists if the socket already exists, am I right?
Fix this by filling in the VNC auto socket path in the same code area
that we fill in default listen address, ports, etc.
https://bugzilla.redhat.com/show_bug.cgi?id=1151762
---
src/qemu/qemu_command.c | 10 ++--------
src/qemu/qemu_process.c | 11 ++++++++++-
2 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ba15dc9..45fc63c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7544,7 +7544,6 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
static int
qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
virCommandPtr cmd,
- virDomainDefPtr def,
virQEMUCapsPtr qemuCaps,
virDomainGraphicsDefPtr graphics)
{
@@ -7561,12 +7560,7 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
goto error;
}
- if (graphics->data.vnc.socket || cfg->vncAutoUnixSocket) {
- if (!graphics->data.vnc.socket &&
- virAsprintf(&graphics->data.vnc.socket,
- "%s/%s.vnc", cfg->libDir, def->name) == -1)
- goto error;
-
+ if (graphics->data.vnc.socket) {
virBufferAsprintf(&opt, "unix:%s", graphics->data.vnc.socket);
} else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNC_COLON)) {
@@ -7944,7 +7938,7 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg,
break;
case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
- return qemuBuildGraphicsVNCCommandLine(cfg, cmd, def, qemuCaps, graphics);
+ return qemuBuildGraphicsVNCCommandLine(cfg, cmd, qemuCaps, graphics);
case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
return qemuBuildGraphicsSPICECommandLine(cfg, cmd, qemuCaps, graphics);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 605b3c6..5de46e2 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4479,7 +4479,16 @@ int qemuProcessStart(virConnectPtr conn,
goto cleanup;
}
- if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC ||
+ if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
+ !graphics->data.vnc.socket &&
+ cfg->vncAutoUnixSocket) {
+ if (virAsprintf(&graphics->data.vnc.socket,
+ "%s/%s.vnc", cfg->libDir, vm->def->name) <
0)
Indentation's off.
+ goto cleanup;
+ }
From the configuration file comment:
"This will only be enabled for VNC configurations that do not have
a hardcoded 'listen' or 'socket' value. This setting takes preference
over vnc_listen."
I see that that's not the true from your code, but it wasn't true even
before. Should we change what we documented when that probably wasn't
ever true at all?
Also I don't see that neither DAC nor SELinux security drivers would
be labelling that vnc socket. Is that happening somewhere else than
in virSecurityManagerSetAllLabel() ?
Even though this is a pretty rare case, it's still a bug and it would
be nice to have it in the release.
+
+ if ((graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
+ !graphics->data.vnc.socket) ||
graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
if (graphics->nListens == 0) {
if (VIR_EXPAND_N(graphics->listens, graphics->nListens, 1) < 0)
--
2.3.6
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
12:08 p.m.
On 04/29/2015 08:32 AM, Martin Kletzander wrote:
On Tue, Apr 28, 2015 at 08:13:19PM -0400, Cole Robinson wrote:
> This can cause permissions failures if qemu.conf user/group is changed.
>
I assume the issue only exists if the socket already exists, am I right?
Hmm, my commit message is bogus. I thought I had hit this issue before and
didn't actually test that it was failing, only verifying that the end result
appeared correct, which it does regardless of this patch. Sorry for the time
wasting.
Here's why it already kinda works without anything explicitly done by libvirt:
you can't bind a unix socket to an existing path, it fails with 'Address
already in use'. qemu handles this by unconditionally unlinking whatever path
is manually passed it. Then the socket bind(2) call will implicitly recreate
the passed in path. In the auto VNC case this effectively causes the
permissions to be reset automagically: qemu can write to the parent
/var/lib/libvirt/qemu directory, and the new socket gets proper user/group
(from the qemu runtime user) and selinux label (from the parent directory)
That said, what _will_ fail is if the user specified an explicit VNC socket
path, but the parent directory was not writeable by qemu. Even if the socket
path is readable/writeable by the user. Kinda confusing but not sure if anyone
cares in practice. FWIW this is true of any usage of qemu's unix: handling.
So this patch doesn't add much... I'll drop it.
There are a few other issues WRT VNC unix handling though:
- Default listen address is still added to the runtime XML, though not to qemu CLI
- VNC sockets are left on disk after qemu shuts down... It's unclear what the
fix is though. Maybe qemu should register an atexit handler to clean up unix
sockets, since its the one that's actually creating them. Or maybe leave it as
it is since it might cause user confusion with manually specific paths.
- Cole
2:38 a.m.
On Wed, Apr 29, 2015 at 01:08:37PM -0400, Cole Robinson wrote:
On 04/29/2015 08:32 AM, Martin Kletzander wrote:
> On Tue, Apr 28, 2015 at 08:13:19PM -0400, Cole Robinson wrote:
>> This can cause permissions failures if qemu.conf user/group is changed.
>>
>
> I assume the issue only exists if the socket already exists, am I right?
>
Hmm, my commit message is bogus. I thought I had hit this issue before and
didn't actually test that it was failing, only verifying that the end result
appeared correct, which it does regardless of this patch. Sorry for the time
wasting.
Here's why it already kinda works without anything explicitly done by libvirt:
you can't bind a unix socket to an existing path, it fails with 'Address
already in use'. qemu handles this by unconditionally unlinking whatever path
is manually passed it. Then the socket bind(2) call will implicitly recreate
the passed in path. In the auto VNC case this effectively causes the
permissions to be reset automagically: qemu can write to the parent
/var/lib/libvirt/qemu directory, and the new socket gets proper user/group
(from the qemu runtime user) and selinux label (from the parent directory)
That said, what _will_ fail is if the user specified an explicit VNC socket
path, but the parent directory was not writeable by qemu. Even if the socket
path is readable/writeable by the user. Kinda confusing but not sure if anyone
cares in practice. FWIW this is true of any usage of qemu's unix: handling.
Since it needs to unlink and create the socket, it must have the
permissions to do so in that respective directory. As with disks
etc., user is responsible for that unless it is auto-generated path.
We cannot change the permissions on all parent directories. It's
definitely worth mentioning that somewhere around the VNC socket stuff
in the documentation.
So this patch doesn't add much... I'll drop it.
There are a few other issues WRT VNC unix handling though:
- Default listen address is still added to the runtime XML, though not to qemu CLI
I remember that when I was checking the code most of the conditions
weren't exclusive, the only thing where using a socket effectively
disabled listening was the command line preparation. I agree that all
the parts should be in sync and not just try initializing and setting
everything unconditionally.
- VNC sockets are left on disk after qemu shuts down... It's
unclear what the
fix is though. Maybe qemu should register an atexit handler to clean up unix
sockets, since its the one that's actually creating them. Or maybe leave it as
it is since it might cause user confusion with manually specific paths.
How is it done with monitor sockets? We are cleaning them in libvirt
if we get the chance, right? That should be the same case with
chardev channel sockets and everything else. Even VNC sockets.
3494
days inactive
3495
days old
3 comments
2 participants
participants (2)
-
Cole Robinson -
Martin Kletzander