[libvirt] [PATCH 0/2] Manage SELinux labels on shared/readonly hostdev's
https://bugzilla.redhat.com/show_bug.cgi?id=1082521 Patch 1 is innocuous and perhaps could have been pushed as trivial... For Patch 2 I wasn't sure if I should use the virSecuritySELinuxSetFilecon or virSecuritySELinuxSetFileconOptional, so I went with the latter since it follows what virSecuritySELinuxSetSecurityImageLabelInternal does. Beyond the check for shared/readonly, the other difference would be for the else condition which uses the Optional now as opposed to the previous code which would call virSecuritySELinuxSetSecurityHostdevLabelHelper and use the non optional call to set the label. John Ferlan (2): tests: Fix sharable typo security: Manage SELinux labels on shared/readonly hostdev's src/security/security_selinux.c | 58 ++++++++++++++++++++++++++++++++++------- tests/qemuargv2xmltest.c | 2 +- 2 files changed, 50 insertions(+), 10 deletions(-) -- 1.9.3
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/qemuargv2xmltest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c index 9c00cd9..cf4a6a8 100644 --- a/tests/qemuargv2xmltest.c +++ b/tests/qemuargv2xmltest.c @@ -30,7 +30,7 @@ static int blankProblemElements(char *data) virtTestClearLineRegex("<currentMemory.*>[[:digit:]]+</currentMemory>", data) < 0 || virtTestClearLineRegex("<readonly/>", data) < 0 || - virtTestClearLineRegex("<sharable/>", data) < 0) + virtTestClearLineRegex("<shareable/>", data) < 0) return -1; return 0; } -- 1.9.3
On 26.11.2014 19:11, John Ferlan wrote:
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/qemuargv2xmltest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c index 9c00cd9..cf4a6a8 100644 --- a/tests/qemuargv2xmltest.c +++ b/tests/qemuargv2xmltest.c @@ -30,7 +30,7 @@ static int blankProblemElements(char *data) virtTestClearLineRegex("<currentMemory.*>[[:digit:]]+</currentMemory>", data) < 0 || virtTestClearLineRegex("<readonly/>", data) < 0 || - virtTestClearLineRegex("<sharable/>", data) < 0) + virtTestClearLineRegex("<shareable/>", data) < 0) return -1; return 0; }
ACK, trivial. Michal
https://bugzilla.redhat.com/show_bug.cgi?id=1082521 Support for shared hostdev's was added in a number of commits, initially starting with 'f2c1d9a80' and most recently commit id 'fd243fc4' to fix issues with the initial implementation. Missed in all those changes was the need to mimic the virSELinux{Set|Restore}SecurityDiskLabel code to handle the "shared" (or shareable) and readonly options when Setting or Restoring the SELinux labels. This patch will adjust the virSecuritySELinuxSetSecuritySCSILabel to not use the virSecuritySELinuxSetSecurityHostdevLabelHelper in order to set the label. Rather follow what the Disk code does by setting the label differently based on whether shareable/readonly is set. This patch will also modify the virSecuritySELinuxRestoreSecuritySCSILabel to follow the same logic as virSecuritySELinuxRestoreSecurityImageLabelInt and not restore the label if shared/readonly Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/security/security_selinux.c | 58 ++++++++++++++++++++++++++++++++++------- 1 file changed, 49 insertions(+), 9 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index f96be50..3a794b8 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -71,6 +71,12 @@ struct _virSecuritySELinuxData { #define SECURITY_SELINUX_VOID_DOI "0" #define SECURITY_SELINUX_NAME "selinux" +/* Data structure to pass to *FileIterate so we have everything we need */ +struct SELinuxSCSICallbackData { + virSecurityManagerPtr mgr; + virDomainDefPtr def; +}; + static int virSecuritySELinuxRestoreSecurityTPMFileLabelInt(virSecurityManagerPtr mgr, virDomainDefPtr def, @@ -1143,7 +1149,7 @@ virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, !src->backingStore) return 0; - /* Don't restore labels on readoly/shared disks, because other VMs may + /* Don't restore labels on readonly/shared disks, because other VMs may * still be accessing these. Alternatively we could iterate over all * running domains and try to figure out if it is in use, but this would * not work for clustered filesystems, since we can't see running VMs using @@ -1309,14 +1315,31 @@ virSecuritySELinuxSetSecurityUSBLabel(virUSBDevicePtr dev ATTRIBUTE_UNUSED, } static int -virSecuritySELinuxSetSecuritySCSILabel(virSCSIDevicePtr dev ATTRIBUTE_UNUSED, +virSecuritySELinuxSetSecuritySCSILabel(virSCSIDevicePtr dev, const char *file, void *opaque) { - return virSecuritySELinuxSetSecurityHostdevLabelHelper(file, opaque); + virSecurityLabelDefPtr secdef; + struct SELinuxSCSICallbackData *ptr = opaque; + virSecurityManagerPtr mgr = ptr->mgr; + virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); + + secdef = virDomainDefGetSecurityLabelDef(ptr->def, SECURITY_SELINUX_NAME); + if (secdef == NULL) + return 0; + + if (virSCSIDeviceGetShareable(dev)) + return virSecuritySELinuxSetFileconOptional(file, + data->file_context); + else if (virSCSIDeviceGetReadonly(dev)) + return virSecuritySELinuxSetFileconOptional(file, + data->content_context); + else + return virSecuritySELinuxSetFileconOptional(file, secdef->imagelabel); } static int -virSecuritySELinuxSetSecurityHostdevSubsysLabel(virDomainDefPtr def, +virSecuritySELinuxSetSecurityHostdevSubsysLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, virDomainHostdevDefPtr dev, const char *vroot) @@ -1377,17 +1400,27 @@ virSecuritySELinuxSetSecurityHostdevSubsysLabel(virDomainDefPtr def, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: { virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; + struct SELinuxSCSICallbackData *ptr; + + if (VIR_ALLOC(ptr) < 0) + goto done; + ptr->mgr = mgr; + ptr->def = def; + virSCSIDevicePtr scsi = virSCSIDeviceNew(NULL, scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit, dev->readonly, dev->shareable); - if (!scsi) + if (!scsi) { + VIR_FREE(ptr); goto done; + } - ret = virSCSIDeviceFileIterate(scsi, virSecuritySELinuxSetSecuritySCSILabel, def); + ret = virSCSIDeviceFileIterate(scsi, virSecuritySELinuxSetSecuritySCSILabel, ptr); virSCSIDeviceFree(scsi); + VIR_FREE(ptr); break; } @@ -1454,7 +1487,7 @@ virSecuritySELinuxSetSecurityHostdevCapsLabel(virDomainDefPtr def, static int -virSecuritySELinuxSetSecurityHostdevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, +virSecuritySELinuxSetSecurityHostdevLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainHostdevDefPtr dev, const char *vroot) @@ -1468,7 +1501,8 @@ virSecuritySELinuxSetSecurityHostdevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UN switch (dev->mode) { case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS: - return virSecuritySELinuxSetSecurityHostdevSubsysLabel(def, dev, vroot); + return virSecuritySELinuxSetSecurityHostdevSubsysLabel(mgr, def, + dev, vroot); case VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES: return virSecuritySELinuxSetSecurityHostdevCapsLabel(def, dev, vroot); @@ -1500,12 +1534,18 @@ virSecuritySELinuxRestoreSecurityUSBLabel(virUSBDevicePtr dev ATTRIBUTE_UNUSED, static int -virSecuritySELinuxRestoreSecuritySCSILabel(virSCSIDevicePtr dev ATTRIBUTE_UNUSED, +virSecuritySELinuxRestoreSecuritySCSILabel(virSCSIDevicePtr dev, const char *file, void *opaque) { virSecurityManagerPtr mgr = opaque; + /* Don't restore labels on a shareable or readonly hostdev, because + * other VMs may still be accessing. + */ + if (virSCSIDeviceGetShareable(dev) || virSCSIDeviceGetReadonly(dev)) + return 0; + return virSecuritySELinuxRestoreSecurityFileLabel(mgr, file); } -- 1.9.3
On 26.11.2014 19:11, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1082521
Support for shared hostdev's was added in a number of commits, initially starting with 'f2c1d9a80' and most recently commit id 'fd243fc4' to fix issues with the initial implementation. Missed in all those changes was the need to mimic the virSELinux{Set|Restore}SecurityDiskLabel code to handle the "shared" (or shareable) and readonly options when Setting or Restoring the SELinux labels.
This patch will adjust the virSecuritySELinuxSetSecuritySCSILabel to not use the virSecuritySELinuxSetSecurityHostdevLabelHelper in order to set the label. Rather follow what the Disk code does by setting the label differently based on whether shareable/readonly is set. This patch will also modify the virSecuritySELinuxRestoreSecuritySCSILabel to follow the same logic as virSecuritySELinuxRestoreSecurityImageLabelInt and not restore the label if shared/readonly
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/security/security_selinux.c | 58 ++++++++++++++++++++++++++++++++++------- 1 file changed, 49 insertions(+), 9 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index f96be50..3a794b8 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -71,6 +71,12 @@ struct _virSecuritySELinuxData { #define SECURITY_SELINUX_VOID_DOI "0" #define SECURITY_SELINUX_NAME "selinux"
+/* Data structure to pass to *FileIterate so we have everything we need */ +struct SELinuxSCSICallbackData { + virSecurityManagerPtr mgr; + virDomainDefPtr def; +}; + static int virSecuritySELinuxRestoreSecurityTPMFileLabelInt(virSecurityManagerPtr mgr, virDomainDefPtr def, @@ -1143,7 +1149,7 @@ virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, !src->backingStore) return 0;
- /* Don't restore labels on readoly/shared disks, because other VMs may + /* Don't restore labels on readonly/shared disks, because other VMs may * still be accessing these. Alternatively we could iterate over all * running domains and try to figure out if it is in use, but this would * not work for clustered filesystems, since we can't see running VMs using @@ -1309,14 +1315,31 @@ virSecuritySELinuxSetSecurityUSBLabel(virUSBDevicePtr dev ATTRIBUTE_UNUSED, }
static int -virSecuritySELinuxSetSecuritySCSILabel(virSCSIDevicePtr dev ATTRIBUTE_UNUSED, +virSecuritySELinuxSetSecuritySCSILabel(virSCSIDevicePtr dev, const char *file, void *opaque) { - return virSecuritySELinuxSetSecurityHostdevLabelHelper(file, opaque); + virSecurityLabelDefPtr secdef; + struct SELinuxSCSICallbackData *ptr = opaque; + virSecurityManagerPtr mgr = ptr->mgr; + virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); + + secdef = virDomainDefGetSecurityLabelDef(ptr->def, SECURITY_SELINUX_NAME); + if (secdef == NULL) + return 0; + + if (virSCSIDeviceGetShareable(dev)) + return virSecuritySELinuxSetFileconOptional(file, + data->file_context); + else if (virSCSIDeviceGetReadonly(dev)) + return virSecuritySELinuxSetFileconOptional(file, + data->content_context); + else + return virSecuritySELinuxSetFileconOptional(file, secdef->imagelabel); }
static int -virSecuritySELinuxSetSecurityHostdevSubsysLabel(virDomainDefPtr def, +virSecuritySELinuxSetSecurityHostdevSubsysLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, virDomainHostdevDefPtr dev, const char *vroot)
@@ -1377,17 +1400,27 @@ virSecuritySELinuxSetSecurityHostdevSubsysLabel(virDomainDefPtr def,
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: { virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; + struct SELinuxSCSICallbackData *ptr;
I think this can be a static variable. I mean, variable allocated on stack, not on the heap.
+ + if (VIR_ALLOC(ptr) < 0) + goto done;
Subsequently, VIR_ALLOC can just go away.
+ ptr->mgr = mgr; + ptr->def = def; + virSCSIDevicePtr scsi = virSCSIDeviceNew(NULL, scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit, dev->readonly, dev->shareable);
- if (!scsi) + if (!scsi) { + VIR_FREE(ptr); goto done; + }
- ret = virSCSIDeviceFileIterate(scsi, virSecuritySELinuxSetSecuritySCSILabel, def); + ret = virSCSIDeviceFileIterate(scsi, virSecuritySELinuxSetSecuritySCSILabel, ptr); virSCSIDeviceFree(scsi); + VIR_FREE(ptr);
And so can VIR_FREE(). ACK with this squashed in: diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 3a794b8..8942f5f 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1400,12 +1400,7 @@ virSecuritySELinuxSetSecurityHostdevSubsysLabel(virSecurityManagerPtr mgr, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: { virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; - struct SELinuxSCSICallbackData *ptr; - - if (VIR_ALLOC(ptr) < 0) - goto done; - ptr->mgr = mgr; - ptr->def = def; + struct SELinuxSCSICallbackData data = {.mgr = mgr, .def = def}; virSCSIDevicePtr scsi = virSCSIDeviceNew(NULL, @@ -1413,14 +1408,11 @@ virSecuritySELinuxSetSecurityHostdevSubsysLabel(virSecurityManagerPtr mgr, scsihostsrc->target, scsihostsrc->unit, dev->readonly, dev->shareable); - if (!scsi) { - VIR_FREE(ptr); + if (!scsi) goto done; - } - ret = virSCSIDeviceFileIterate(scsi, virSecuritySELinuxSetSecuritySCSILabel, ptr); + ret = virSCSIDeviceFileIterate(scsi, virSecuritySELinuxSetSecuritySCSILabel, &data); virSCSIDeviceFree(scsi); - VIR_FREE(ptr); break; } Michal
ping... Tks, John On 11/26/2014 01:11 PM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1082521
Patch 1 is innocuous and perhaps could have been pushed as trivial...
For Patch 2 I wasn't sure if I should use the virSecuritySELinuxSetFilecon or virSecuritySELinuxSetFileconOptional, so I went with the latter since it follows what virSecuritySELinuxSetSecurityImageLabelInternal does. Beyond the check for shared/readonly, the other difference would be for the else condition which uses the Optional now as opposed to the previous code which would call virSecuritySELinuxSetSecurityHostdevLabelHelper and use the non optional call to set the label.
John Ferlan (2): tests: Fix sharable typo security: Manage SELinux labels on shared/readonly hostdev's
src/security/security_selinux.c | 58 ++++++++++++++++++++++++++++++++++------- tests/qemuargv2xmltest.c | 2 +- 2 files changed, 50 insertions(+), 10 deletions(-)
On 26.11.2014 19:11, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1082521
Patch 1 is innocuous and perhaps could have been pushed as trivial...
For Patch 2 I wasn't sure if I should use the virSecuritySELinuxSetFilecon or virSecuritySELinuxSetFileconOptional, so I went with the latter since it follows what virSecuritySELinuxSetSecurityImageLabelInternal does. Beyond the check for shared/readonly, the other difference would be for the else condition which uses the Optional now as opposed to the previous code which would call virSecuritySELinuxSetSecurityHostdevLabelHelper and use the non optional call to set the label.
John Ferlan (2): tests: Fix sharable typo security: Manage SELinux labels on shared/readonly hostdev's
src/security/security_selinux.c | 58 ++++++++++++++++++++++++++++++++++------- tests/qemuargv2xmltest.c | 2 +- 2 files changed, 50 insertions(+), 10 deletions(-)
Oh, now that we are in the freeze I should explicitly state that this is safe to push during the freeze - it's a bug fix, not a feature. Michal
On 12/09/2014 04:37 AM, Michal Privoznik wrote:
On 26.11.2014 19:11, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1082521
Patch 1 is innocuous and perhaps could have been pushed as trivial...
For Patch 2 I wasn't sure if I should use the virSecuritySELinuxSetFilecon or virSecuritySELinuxSetFileconOptional, so I went with the latter since it follows what virSecuritySELinuxSetSecurityImageLabelInternal does. Beyond the check for shared/readonly, the other difference would be for the else condition which uses the Optional now as opposed to the previous code which would call virSecuritySELinuxSetSecurityHostdevLabelHelper and use the non optional call to set the label.
John Ferlan (2): tests: Fix sharable typo security: Manage SELinux labels on shared/readonly hostdev's
src/security/security_selinux.c | 58 ++++++++++++++++++++++++++++++++++------- tests/qemuargv2xmltest.c | 2 +- 2 files changed, 50 insertions(+), 10 deletions(-)
Oh, now that we are in the freeze I should explicitly state that this is safe to push during the freeze - it's a bug fix, not a feature.
Michal
OK - thanks. Made the adjustments requested and pushed. John
participants (2)
-
John Ferlan -
Michal Privoznik