[libvirt] [PATCH] Fix starting qemu instances when apparmor driver is enabled
With the apparmor security driver enabled, qemu instances fail to start # grep ^security_driver /etc/libvirt/qemu.conf security_driver = "apparmor" # virsh start test-kvm error: Failed to start domain test-kvm error: internal error security label already defined for VM The model field of virSecurityLabelDef object is always populated by virDomainDefGetSecurityLabelDef(), so remove the check for a NULL model when verifying if a label is already defined for the instance. Checking for a NULL model and populating it later in AppArmorGenSecurityLabel() has been left in the code to be consistent with virSecuritySELinuxGenSecurityLabel(). --- src/security/security_apparmor.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index ddc1fe4..2e6a57f 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -436,8 +436,7 @@ AppArmorGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return rc; } - if ((secdef->label) || - (secdef->model) || (secdef->imagelabel)) { + if (secdef->label || secdef->imagelabel) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("security label already defined for VM")); @@ -461,8 +460,7 @@ AppArmorGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, goto err; } - secdef->model = strdup(SECURITY_APPARMOR_NAME); - if (!secdef->model) { + if (!secdef->model && !(secdef->model = strdup(SECURITY_APPARMOR_NAME))) { virReportOOMError(); goto err; } -- 1.8.0.1
On 02/27/2013 04:51 PM, Jim Fehlig wrote:
With the apparmor security driver enabled, qemu instances fail to start
# grep ^security_driver /etc/libvirt/qemu.conf security_driver = "apparmor" # virsh start test-kvm error: Failed to start domain test-kvm error: internal error security label already defined for VM
The model field of virSecurityLabelDef object is always populated by virDomainDefGetSecurityLabelDef(), so remove the check for a NULL model when verifying if a label is already defined for the instance.
Checking for a NULL model and populating it later in AppArmorGenSecurityLabel() has been left in the code to be consistent with virSecuritySELinuxGenSecurityLabel(). --- src/security/security_apparmor.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
ACK; and safe for 1.0.3. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 02/27/2013 04:51 PM, Jim Fehlig wrote:
With the apparmor security driver enabled, qemu instances fail to start
# grep ^security_driver /etc/libvirt/qemu.conf security_driver = "apparmor" # virsh start test-kvm error: Failed to start domain test-kvm error: internal error security label already defined for VM
The model field of virSecurityLabelDef object is always populated by virDomainDefGetSecurityLabelDef(), so remove the check for a NULL model when verifying if a label is already defined for the instance.
Checking for a NULL model and populating it later in AppArmorGenSecurityLabel() has been left in the code to be consistent with virSecuritySELinuxGenSecurityLabel(). --- src/security/security_apparmor.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
ACK; and safe for 1.0.3.
Thanks, pushed now.
On 03/01/2013 08:37 AM, Jim Fehlig wrote:
Eric Blake wrote:
On 02/27/2013 04:51 PM, Jim Fehlig wrote:
With the apparmor security driver enabled, qemu instances fail to start
# grep ^security_driver /etc/libvirt/qemu.conf security_driver = "apparmor" # virsh start test-kvm error: Failed to start domain test-kvm error: internal error security label already defined for VM
The model field of virSecurityLabelDef object is always populated by virDomainDefGetSecurityLabelDef(), so remove the check for a NULL model when verifying if a label is already defined for the instance.
Checking for a NULL model and populating it later in AppArmorGenSecurityLabel() has been left in the code to be consistent with virSecuritySELinuxGenSecurityLabel(). --- src/security/security_apparmor.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
ACK; and safe for 1.0.3.
Thanks, pushed now.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Hi Jim In selinux, libvirt added a label for tapfd. Do you think this patch makes sense for apparmor? https://www.redhat.com/archives/libvir-list/2012-October/msg01461.html Gunannan
Guannan Ren wrote:
On 03/01/2013 08:37 AM, Jim Fehlig wrote:
Eric Blake wrote:
On 02/27/2013 04:51 PM, Jim Fehlig wrote:
With the apparmor security driver enabled, qemu instances fail to start
# grep ^security_driver /etc/libvirt/qemu.conf security_driver = "apparmor" # virsh start test-kvm error: Failed to start domain test-kvm error: internal error security label already defined for VM
The model field of virSecurityLabelDef object is always populated by virDomainDefGetSecurityLabelDef(), so remove the check for a NULL model when verifying if a label is already defined for the instance.
Checking for a NULL model and populating it later in AppArmorGenSecurityLabel() has been left in the code to be consistent with virSecuritySELinuxGenSecurityLabel(). --- src/security/security_apparmor.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
ACK; and safe for 1.0.3.
Thanks, pushed now.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Hi Jim
In selinux, libvirt added a label for tapfd. Do you think this patch makes sense for apparmor? https://www.redhat.com/archives/libvir-list/2012-October/msg01461.html
Hi Gunannan, Apologies for missing your initial post of that series. I see that you fixed this exact bug in 2/3 :(. I think 3/3 does make sense for apparmor, but I'm not sure about using AppArmorSetImageFDLabel() as a common function. It returns if secdef->imagelabel == NULL, which would be incorrect if labeling a tap fd right? I promise not to miss the patch if you respin it :). Regards, Jim
On 03/02/2013 12:41 AM, Jim Fehlig wrote:
Guannan Ren wrote:
Hi Jim
In selinux, libvirt added a label for tapfd. Do you think this patch makes sense for apparmor? https://www.redhat.com/archives/libvir-list/2012-October/msg01461.html
Hi Gunannan,
Apologies for missing your initial post of that series. I see that you fixed this exact bug in 2/3 :(.
I think 3/3 does make sense for apparmor, but I'm not sure about using AppArmorSetImageFDLabel() as a common function. It returns if secdef->imagelabel == NULL, which would be incorrect if labeling a tap fd right?
I promise not to miss the patch if you respin it :).
Regards, Jim
Nothing to apologize, I really don't know much about apparmor. The tapfd I mean here is not used by libvirt deamon, it is a tapfd created on particular guest which is using macvtap driver to attach virtual NIC to a given physical interface. From the code, the secdef->imagelabel have the same value as secdef->label which is libvirt-{uuid} file in /etc/apparmor.d/libvirt folder. If it is null, that means the guest will not be confined by apparmor, so is this tapfd, I think this is fine. If you think it is reasonable, I will rebase that patch and send a v2. Guannan
Guannan Ren wrote:
On 03/02/2013 12:41 AM, Jim Fehlig wrote:
Guannan Ren wrote:
Hi Jim
In selinux, libvirt added a label for tapfd. Do you think this patch makes sense for apparmor? https://www.redhat.com/archives/libvir-list/2012-October/msg01461.html
Hi Gunannan,
Apologies for missing your initial post of that series. I see that you fixed this exact bug in 2/3 :(.
I think 3/3 does make sense for apparmor, but I'm not sure about using AppArmorSetImageFDLabel() as a common function. It returns if secdef->imagelabel == NULL, which would be incorrect if labeling a tap fd right?
I promise not to miss the patch if you respin it :).
Regards, Jim
Nothing to apologize, I really don't know much about apparmor. The tapfd I mean here is not used by libvirt deamon, it is a tapfd created on particular guest which is using macvtap driver to attach virtual NIC to a given physical interface. From the code, the secdef->imagelabel have the same value as secdef->label which is libvirt-{uuid} file in /etc/apparmor.d/libvirt folder. If it is null, that means the guest will not be confined by apparmor, so is this tapfd, I think this is fine.
Yes, agreed.
If you think it is reasonable, I will rebase that patch and send a v2.
Yep, I think it is reasonable and necessary. I finally got around to testing your patch and it is indeed needed when using macvtap with apparmor-confined guests. Thanks for looking into this! Regards, Jim
Rename AppArmorSetImageFDLabel to AppArmorSetFDLabel which could be used as a common function for *ALL* fd relabelling in Linux. In apparmor profile for specific vm with uuid cdbebdfa-1d6d-65c3-be0f-fd74b978a773 Path: /etc/apparmor.d/libvirt/libvirt-cdbebdfa-1d6d-65c3-be0f-fd74b978a773.files The last line is for the tapfd relabelling. # DO NOT EDIT THIS FILE DIRECTLY. IT IS MANAGED BY LIBVIRT. "/var/log/libvirt/**/rhel6qcow2.log" w, "/var/lib/libvirt/**/rhel6qcow2.monitor" rw, "/var/run/libvirt/**/rhel6qcow2.pid" rwk, "/run/libvirt/**/rhel6qcow2.pid" rwk, "/var/run/libvirt/**/*.tunnelmigrate.dest.rhel6qcow2" rw, "/run/libvirt/**/*.tunnelmigrate.dest.rhel6qcow2" rw, "/var/lib/libvirt/images/rhel6u3qcow2.img" rw, "/dev/tap45" rw, --- src/security/security_apparmor.c | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 2e6a57f..9dd8d74 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -884,9 +884,9 @@ AppArmorRestoreSavedStateLabel(virSecurityManagerPtr mgr, } static int -AppArmorSetImageFDLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - int fd) +AppArmorSetFDLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + int fd) { int rc = -1; char *proc = NULL; @@ -915,16 +915,6 @@ AppArmorSetImageFDLabel(virSecurityManagerPtr mgr, return reload_profile(mgr, def, fd_path, true); } -/* TODO need code here */ -static int -AppArmorSetTapFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr def ATTRIBUTE_UNUSED, - int fd ATTRIBUTE_UNUSED) -{ - return 0; -} - - static char * AppArmorGetMountOptions(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr vm ATTRIBUTE_UNUSED) @@ -975,8 +965,8 @@ virSecurityDriver virAppArmorSecurityDriver = { .domainSetSavedStateLabel = AppArmorSetSavedStateLabel, .domainRestoreSavedStateLabel = AppArmorRestoreSavedStateLabel, - .domainSetSecurityImageFDLabel = AppArmorSetImageFDLabel, - .domainSetSecurityTapFDLabel = AppArmorSetTapFDLabel, + .domainSetSecurityImageFDLabel = AppArmorSetFDLabel, + .domainSetSecurityTapFDLabel = AppArmorSetFDLabel, .domainGetSecurityMountOptions = AppArmorGetMountOptions, }; -- 1.7.11.2
Guannan Ren wrote:
Rename AppArmorSetImageFDLabel to AppArmorSetFDLabel which could be used as a common function for *ALL* fd relabelling in Linux.
In apparmor profile for specific vm with uuid cdbebdfa-1d6d-65c3-be0f-fd74b978a773 Path: /etc/apparmor.d/libvirt/libvirt-cdbebdfa-1d6d-65c3-be0f-fd74b978a773.files The last line is for the tapfd relabelling.
# DO NOT EDIT THIS FILE DIRECTLY. IT IS MANAGED BY LIBVIRT. "/var/log/libvirt/**/rhel6qcow2.log" w, "/var/lib/libvirt/**/rhel6qcow2.monitor" rw, "/var/run/libvirt/**/rhel6qcow2.pid" rwk, "/run/libvirt/**/rhel6qcow2.pid" rwk, "/var/run/libvirt/**/*.tunnelmigrate.dest.rhel6qcow2" rw, "/run/libvirt/**/*.tunnelmigrate.dest.rhel6qcow2" rw, "/var/lib/libvirt/images/rhel6u3qcow2.img" rw, "/dev/tap45" rw, --- src/security/security_apparmor.c | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-)
ACK.
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 2e6a57f..9dd8d74 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -884,9 +884,9 @@ AppArmorRestoreSavedStateLabel(virSecurityManagerPtr mgr, }
static int -AppArmorSetImageFDLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - int fd) +AppArmorSetFDLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + int fd) { int rc = -1; char *proc = NULL; @@ -915,16 +915,6 @@ AppArmorSetImageFDLabel(virSecurityManagerPtr mgr, return reload_profile(mgr, def, fd_path, true); }
-/* TODO need code here */ -static int -AppArmorSetTapFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr def ATTRIBUTE_UNUSED, - int fd ATTRIBUTE_UNUSED) -{ - return 0; -} - - static char * AppArmorGetMountOptions(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr vm ATTRIBUTE_UNUSED) @@ -975,8 +965,8 @@ virSecurityDriver virAppArmorSecurityDriver = { .domainSetSavedStateLabel = AppArmorSetSavedStateLabel, .domainRestoreSavedStateLabel = AppArmorRestoreSavedStateLabel,
- .domainSetSecurityImageFDLabel = AppArmorSetImageFDLabel, - .domainSetSecurityTapFDLabel = AppArmorSetTapFDLabel, + .domainSetSecurityImageFDLabel = AppArmorSetFDLabel, + .domainSetSecurityTapFDLabel = AppArmorSetFDLabel,
.domainGetSecurityMountOptions = AppArmorGetMountOptions, };
On 03/08/2013 05:21 AM, Jim Fehlig wrote:
Guannan Ren wrote:
Rename AppArmorSetImageFDLabel to AppArmorSetFDLabel which could be used as a common function for *ALL* fd relabelling in Linux.
In apparmor profile for specific vm with uuid cdbebdfa-1d6d-65c3-be0f-fd74b978a773 Path: /etc/apparmor.d/libvirt/libvirt-cdbebdfa-1d6d-65c3-be0f-fd74b978a773.files The last line is for the tapfd relabelling.
# DO NOT EDIT THIS FILE DIRECTLY. IT IS MANAGED BY LIBVIRT. "/var/log/libvirt/**/rhel6qcow2.log" w, "/var/lib/libvirt/**/rhel6qcow2.monitor" rw, "/var/run/libvirt/**/rhel6qcow2.pid" rwk, "/run/libvirt/**/rhel6qcow2.pid" rwk, "/var/run/libvirt/**/*.tunnelmigrate.dest.rhel6qcow2" rw, "/run/libvirt/**/*.tunnelmigrate.dest.rhel6qcow2" rw, "/var/lib/libvirt/images/rhel6u3qcow2.img" rw, "/dev/tap45" rw, --- src/security/security_apparmor.c | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-)
ACK.
Thanks, pushed. Guannan
participants (3)
-
Eric Blake -
Guannan Ren -
Jim Fehlig