9:55 a.m.
Actually, 2/2 suggests we need to tweak SELinux policy too. Should I
file a bug?
Michal Prívozník (2):
qemu_shim: Allow other users to enter the root dir
qemu_shim: Ignore SIGPIPE
src/qemu/qemu_shim.c | 7 +++++++
1 file changed, 7 insertions(+)
--
2.24.1
9:56 a.m.
New subject: [PATCH 1/2] qemu_shim: Allow other users to enter the root dir
When virt-qemu-run is ran without any root directory specified on
the command line, a temporary directory is made and used instead.
But since we are using g_dir_make_tmp() to create the directory
it is going to have 0700 mode. So even though we create the whole
directory structure under it and label everything, QEMU is very
likely to not have the access. This is because in this case there
is no qemu.conf and thus distro default UID:GID is used to run
QEMU (e.g. qemu:kvm on Fedora). Change the mode of the temporary
directory so that everybody has eXecute permission.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_shim.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_shim.c b/src/qemu/qemu_shim.c
index 5b7840e971..4f06ae952c 100644
--- a/src/qemu/qemu_shim.c
+++ b/src/qemu/qemu_shim.c
@@ -158,6 +158,12 @@ int main(int argc, char **argv)
return 1;
}
tmproot = true;
+
+ if (chmod(root, S_IRWXU | S_IXGRP | S_IXOTH) < 0) {
+ g_printerr("%s: cannot chown temporary dir: %s\n",
+ argv[0], g_strerror(errno));
+ goto cleanup;
+ }
}
virFileActivateDirOverrideForProg(argv[0]);
--
2.24.1
3 a.m.
New subject: [PATCH 1/2] qemu_shim: Allow other users to enter the root dir
On Fri, 2020-02-28 at 16:56 +0100, Michal Privoznik wrote:
+++ b/src/qemu/qemu_shim.c
@@ -158,6 +158,12 @@ int main(int argc, char **argv)
return 1;
}
tmproot = true;
+
+ if (chmod(root, S_IRWXU | S_IXGRP | S_IXOTH) < 0) {
I think this is unnecessarily restrictive: the directories that are
created right underneath root are all 0755, with the files themselves
being mostly 0600, so using 0711 here is only going to add a bit of
annoyance rather than actual security I think.
Also, and this is a personal preference so feel free to ignore it,
I would find using octal values directly more readable.
With a more permissive mode used,
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
9:56 a.m.
New subject: [PATCH 2/2] qemu_shim: Ignore SIGPIPE
I've found that if my virtlogd is socket activated but the daemon
doesn't run yet, then the virt-qemu-run is killed right after it
tries to start the domain. The problem is that because the default
setting is to use virtlogd, the domain create code tries to
connect to virtlogd socket, which in turn tries to detect who is
connecting (virNetSocketGetUNIXIdentity()) and as a part of it,
it will try to open /proc/${PID_OF_SHIM}/stat which is denied by
SELinux:
type=AVC msg=audit(1582903501.927:323): avc: denied { search } for \
pid=1210 comm="virtlogd" name="1843" dev="proc" ino=37224
\
scontext=system_u:system_r:virtlogd_t:s0-s0:c0.c1023 \
tcontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tclass=dir \
permissive=0
Virtlogd reacts by closing the connection which the shim sees as
SIGPIPE. Since the default response to the signal is Term, we
don't even get to reporting any error nor to removing the
temporary directory.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_shim.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_shim.c b/src/qemu/qemu_shim.c
index 4f06ae952c..d8f3902874 100644
--- a/src/qemu/qemu_shim.c
+++ b/src/qemu/qemu_shim.c
@@ -150,6 +150,7 @@ int main(int argc, char **argv)
signal(SIGINT, qemuShimSigShutdown);
signal(SIGQUIT, qemuShimSigShutdown);
signal(SIGHUP, qemuShimSigShutdown);
+ signal(SIGPIPE, SIG_IGN);
if (root == NULL) {
if (!(root = g_dir_make_tmp("virt-qemu-run-XXXXXX", &error))) {
--
2.24.1
3:06 a.m.
New subject: [PATCH 2/2] qemu_shim: Ignore SIGPIPE
On Fri, 2020-02-28 at 16:56 +0100, Michal Privoznik wrote:
I've found that if my virtlogd is socket activated but the
daemon
doesn't run yet, then the virt-qemu-run is killed right after it
tries to start the domain. The problem is that because the default
setting is to use virtlogd, the domain create code tries to
connect to virtlogd socket, which in turn tries to detect who is
connecting (virNetSocketGetUNIXIdentity()) and as a part of it,
it will try to open /proc/${PID_OF_SHIM}/stat which is denied by
SELinux:
type=AVC msg=audit(1582903501.927:323): avc: denied { search } for \
pid=1210 comm="virtlogd" name="1843" dev="proc" ino=37224
\
scontext=system_u:system_r:virtlogd_t:s0-s0:c0.c1023 \
tcontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tclass=dir \
permissive=0
Virtlogd reacts by closing the connection which the shim sees as
SIGPIPE. Since the default response to the signal is Term, we
don't even get to reporting any error nor to removing the
temporary directory.
While I've been hitting this communication issue with virtlogd
consistently, I haven't been able to reproduce the exact sympthoms
you list: more specifically, the AVC doesn't show up in audit.log,
and virt-qemu-run not only reports the error but successfully cleans
up after itself.
[...]
+++ b/src/qemu/qemu_shim.c
@@ -150,6 +150,7 @@ int main(int argc, char **argv)
signal(SIGINT, qemuShimSigShutdown);
signal(SIGQUIT, qemuShimSigShutdown);
signal(SIGHUP, qemuShimSigShutdown);
+ signal(SIGPIPE, SIG_IGN);
Either way, I'm not convinced this is the right fix: if virt-qemu-run
is unable to communicate with virtlogd, that is a serious issue that
should prevent the application from continuing. Or does this change
only make it so virt-qemu-run does not abort immediately but rather
gets far enough that it can report the error and clean up? Again, not
being able to reproduce the original issue locally makes it difficult
to validate the fix :)
--
Andrea Bolognani / Red Hat / Virtualization
3:57 a.m.
New subject: [PATCH 2/2] qemu_shim: Ignore SIGPIPE
On Thu, Mar 05, 2020 at 10:06:53AM +0100, Andrea Bolognani wrote:
On Fri, 2020-02-28 at 16:56 +0100, Michal Privoznik wrote:
> I've found that if my virtlogd is socket activated but the daemon
> doesn't run yet, then the virt-qemu-run is killed right after it
> tries to start the domain. The problem is that because the default
> setting is to use virtlogd, the domain create code tries to
> connect to virtlogd socket, which in turn tries to detect who is
> connecting (virNetSocketGetUNIXIdentity()) and as a part of it,
> it will try to open /proc/${PID_OF_SHIM}/stat which is denied by
> SELinux:
>
> type=AVC msg=audit(1582903501.927:323): avc: denied { search } for \
> pid=1210 comm="virtlogd" name="1843" dev="proc"
ino=37224 \
> scontext=system_u:system_r:virtlogd_t:s0-s0:c0.c1023 \
> tcontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tclass=dir \
> permissive=0
>
> Virtlogd reacts by closing the connection which the shim sees as
> SIGPIPE. Since the default response to the signal is Term, we
> don't even get to reporting any error nor to removing the
> temporary directory.
While I've been hitting this communication issue with virtlogd
consistently, I haven't been able to reproduce the exact sympthoms
you list: more specifically, the AVC doesn't show up in audit.log,
and virt-qemu-run not only reports the error but successfully cleans
up after itself.
[...]
> +++ b/src/qemu/qemu_shim.c
> @@ -150,6 +150,7 @@ int main(int argc, char **argv)
> signal(SIGINT, qemuShimSigShutdown);
> signal(SIGQUIT, qemuShimSigShutdown);
> signal(SIGHUP, qemuShimSigShutdown);
> + signal(SIGPIPE, SIG_IGN);
Either way, I'm not convinced this is the right fix: if virt-qemu-run
is unable to communicate with virtlogd, that is a serious issue that
should prevent the application from continuing. Or does this change
only make it so virt-qemu-run does not abort immediately but rather
gets far enough that it can report the error and clean up? Again, not
being able to reproduce the original issue locally makes it difficult
to validate the fix :)
Regardless of the actual problem faced, ignoring SIGPIPE is the right
thing todo for all programs. They'll then process normal error returns
from write()
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
4:02 a.m.
New subject: [PATCH 2/2] qemu_shim: Ignore SIGPIPE
On Thu, 2020-03-05 at 09:57 +0000, Daniel P. Berrangé wrote:
On Thu, Mar 05, 2020 at 10:06:53AM +0100, Andrea Bolognani wrote:
> On Fri, 2020-02-28 at 16:56 +0100, Michal Privoznik wrote:
> > +++ b/src/qemu/qemu_shim.c
> > @@ -150,6 +150,7 @@ int main(int argc, char **argv)
> > signal(SIGINT, qemuShimSigShutdown);
> > signal(SIGQUIT, qemuShimSigShutdown);
> > signal(SIGHUP, qemuShimSigShutdown);
> > + signal(SIGPIPE, SIG_IGN);
>
> Either way, I'm not convinced this is the right fix: if virt-qemu-run
> is unable to communicate with virtlogd, that is a serious issue that
> should prevent the application from continuing. Or does this change
> only make it so virt-qemu-run does not abort immediately but rather
> gets far enough that it can report the error and clean up? Again, not
> being able to reproduce the original issue locally makes it difficult
> to validate the fix :)
Regardless of the actual problem faced, ignoring SIGPIPE is the right
thing todo for all programs. They'll then process normal error returns
from write()
The more you know!
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
then.
--
Andrea Bolognani / Red Hat / Virtualization
1814
days inactive
1820
days old
6 comments
3 participants
participants (3)
-
Andrea Bolognani -
Daniel P. Berrangé -
Michal Privoznik