[libvirt] [PATCH] security: Do not restore kernel and initrd labels
Kernel/initrd files are essentially read-only shareable images and thus should be handled in the same way. We already use the appropriate label for kernel/initrd files when starting a domain, but when a domain gets destroyed we would remove the labels which would make other running domains using the same files very unhappy. https://bugzilla.redhat.com/show_bug.cgi?id=921135 Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/security/security_dac.c | 8 -------- src/security/security_selinux.c | 8 -------- 2 files changed, 16 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 80709fe..378b922 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1128,14 +1128,6 @@ virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr, virSecurityDACRestoreFileLabel(priv, def->os.loader->nvram) < 0) rc = -1; - if (def->os.kernel && - virSecurityDACRestoreFileLabel(priv, def->os.kernel) < 0) - rc = -1; - - if (def->os.initrd && - virSecurityDACRestoreFileLabel(priv, def->os.initrd) < 0) - rc = -1; - if (def->os.dtb && virSecurityDACRestoreFileLabel(priv, def->os.dtb) < 0) rc = -1; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 721c451..475cdbc 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2034,14 +2034,6 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr mgr, virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram) < 0) rc = -1; - if (def->os.kernel && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.kernel) < 0) - rc = -1; - - if (def->os.initrd && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.initrd) < 0) - rc = -1; - if (def->os.dtb && virSecuritySELinuxRestoreFileLabel(mgr, def->os.dtb) < 0) rc = -1; -- 2.7.0
On Fri, Jan 15, 2016 at 11:11:18AM +0100, Jiri Denemark wrote:
Kernel/initrd files are essentially read-only shareable images and thus should be handled in the same way. We already use the appropriate label for kernel/initrd files when starting a domain, but when a domain gets destroyed we would remove the labels which would make other running domains using the same files very unhappy.
https://bugzilla.redhat.com/show_bug.cgi?id=921135
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/security/security_dac.c | 8 -------- src/security/security_selinux.c | 8 -------- 2 files changed, 16 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 80709fe..378b922 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1128,14 +1128,6 @@ virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr, virSecurityDACRestoreFileLabel(priv, def->os.loader->nvram) < 0) rc = -1;
- if (def->os.kernel && - virSecurityDACRestoreFileLabel(priv, def->os.kernel) < 0) - rc = -1; - - if (def->os.initrd && - virSecurityDACRestoreFileLabel(priv, def->os.initrd) < 0) - rc = -1; - if (def->os.dtb && virSecurityDACRestoreFileLabel(priv, def->os.dtb) < 0) rc = -1; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 721c451..475cdbc 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2034,14 +2034,6 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr mgr, virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram) < 0) rc = -1;
- if (def->os.kernel && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.kernel) < 0) - rc = -1; - - if (def->os.initrd && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.initrd) < 0) - rc = -1; - if (def->os.dtb && virSecuritySELinuxRestoreFileLabel(mgr, def->os.dtb) < 0) rc = -1;
ACK but I'm wondering if the nvram and dtb lines before & after would potentially suffer the same problem Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Fri, Jan 15, 2016 at 10:23:03 +0000, Daniel P. Berrange wrote:
On Fri, Jan 15, 2016 at 11:11:18AM +0100, Jiri Denemark wrote:
Kernel/initrd files are essentially read-only shareable images and thus should be handled in the same way. We already use the appropriate label for kernel/initrd files when starting a domain, but when a domain gets destroyed we would remove the labels which would make other running domains using the same files very unhappy.
https://bugzilla.redhat.com/show_bug.cgi?id=921135
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
ACK
Thanks, I pushed the patch.
but I'm wondering if the nvram and dtb lines before & after would potentially suffer the same problem
Yeah, I was wondering about that too, but I wasn't quite sure whether they are similar or not. Jirka
On 15 Jan 2016, at 10:31, Jiri Denemark <jdenemar@redhat.com> wrote:
On Fri, Jan 15, 2016 at 10:23:03 +0000, Daniel P. Berrange wrote:
On Fri, Jan 15, 2016 at 11:11:18AM +0100, Jiri Denemark wrote:
Kernel/initrd files are essentially read-only shareable images and thus should be handled in the same way. We already use the appropriate label for kernel/initrd files when starting a domain, but when a domain gets destroyed we would remove the labels which would make other running domains using the same files very unhappy.
https://bugzilla.redhat.com/show_bug.cgi?id=921135
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
ACK
Thanks, I pushed the patch.
but I'm wondering if the nvram and dtb lines before & after would potentially suffer the same problem
Yeah, I was wondering about that too, but I wasn't quite sure whether they are similar or not.
Could Rich's test be tweaked some way in order to find out? + Justin
On Fri, Jan 15, 2016 at 10:44:30 +0000, Justin Clift wrote:
On 15 Jan 2016, at 10:31, Jiri Denemark <jdenemar@redhat.com> wrote:
On Fri, Jan 15, 2016 at 10:23:03 +0000, Daniel P. Berrange wrote:
On Fri, Jan 15, 2016 at 11:11:18AM +0100, Jiri Denemark wrote:
Kernel/initrd files are essentially read-only shareable images and thus should be handled in the same way. We already use the appropriate label for kernel/initrd files when starting a domain, but when a domain gets destroyed we would remove the labels which would make other running domains using the same files very unhappy.
https://bugzilla.redhat.com/show_bug.cgi?id=921135
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
ACK
Thanks, I pushed the patch.
but I'm wondering if the nvram and dtb lines before & after would potentially suffer the same problem
Yeah, I was wondering about that too, but I wasn't quite sure whether they are similar or not.
Could Rich's test be tweaked some way in order to find out?
Well, it could, but the question is whether it would be correct usage :-) And it seems nvram is actually different: /* This is different than kernel or initrd. The nvram store * is really a disk, qemu can read and write to it. */ and we use imagelabel for nvram. However, dtb (whatever that is used for) gets the same label we use for kernel/initrd so it looks like it could be similar. However, I have no idea what this beast is all about :-) Jirka
On Fri, 2016-01-15 at 11:50 +0100, Jiri Denemark wrote:
but I'm wondering if the nvram and dtb lines before & after would potentially suffer the same problem Yeah, I was wondering about that too, but I wasn't quite sure whether they are similar or not. Could Rich's test be tweaked some way in order to find out? Well, it could, but the question is whether it would be correct usage :-) And it seems nvram is actually different: /* This is different than kernel or initrd. The nvram store * is really a disk, qemu can read and write to it. */ and we use imagelabel for nvram. However, dtb (whatever that is used for) gets the same label we use for kernel/initrd so it looks like it could be similar. However, I have no idea what this beast is all about :-)
AFAICT the Device Tree (which is contained in the file pointed to by the <dtb> element) is copied into the guest memory at startup and only used by the kernel to collect information about the hardware, so it should be safe to treat it the same way as kernel and initrd. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
participants (4)
-
Andrea Bolognani -
Daniel P. Berrange -
Jiri Denemark -
Justin Clift