On 12/2/19 9:26 AM, Michal Privoznik wrote:
> This function is currently not called for any type of storage
> source that is not considered 'local' (as defined by
> virStorageSourceIsLocalStorage()). Well, NVMe disks are not
> 'local' from that point of view and therefore we will need to
> call this function more frequently.
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> src/security/security_apparmor.c | 25 ++++++++++++----
> src/security/security_dac.c | 30 +++++++++++++++++++
> src/security/security_selinux.c | 51 +++++++++++++++++++++++++++-----
> 3 files changed, 92 insertions(+), 14 deletions(-)
>
> diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
> index 21560b2330..7cea788931 100644
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -779,9 +779,8 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
> virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED)
> {
> virSecurityLabelDefPtr secdef;
> -
> - if (!src->path || !virStorageSourceIsLocalStorage(src))
> - return 0;
> + g_autofree char *vfioGroupDev = NULL;
> + const char *path;
>
> secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME);
> if (!secdef || !secdef->relabel)
> @@ -790,15 +789,29 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
> if (!secdef->imagelabel)
> return 0;
>
> + if (src->type == VIR_STORAGE_TYPE_NVME) {
> + const virStorageSourceNVMeDef *nvme = src->nvme;
> +
> + if (!(vfioGroupDev = virPCIDeviceAddressGetIOMMUGroupDev(&nvme->pciAddr)))
> + return -1;
> +
> + path = vfioGroupDev;
> + } else {
> + if (!src->path || !virStorageSourceIsLocalStorage(src))
> + return 0;
> +
> + path = src->path;
> + }
> +
> /* if the device doesn't exist, error out */
> - if (!virFileExists(src->path)) {
> + if (!virFileExists(path)) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("\'%s\' does not exist"),
> - src->path);
> + path);
> return -1;
> }
>
> - return reload_profile(mgr, def, src->path, true);
> + return reload_profile(mgr, def, path, true);
> }
>
> static int
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index a9a1fad5d7..411aa1b159 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -911,6 +911,19 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr,
> return -1;
> }
>
> + /* This is not very clean. But so far we don't have NVMe
> + * storage pool backend so that its chownCallback would be
> + * called. And this place looks least offensive. */
> + if (src->type == VIR_STORAGE_TYPE_NVME) {
> + const virStorageSourceNVMeDef *nvme = src->nvme;
> + g_autofree char *vfioGroupDev = NULL;
> +
> + if (!(vfioGroupDev = virPCIDeviceAddressGetIOMMUGroupDev(&nvme->pciAddr)))
> + return -1;
> +
> + return virSecurityDACSetOwnership(mgr, NULL, vfioGroupDev, user, group, false);
> + }
> +
> /* We can't do restore on shared resources safely. Not even
> * with refcounting implemented in XATTRs because if there
> * was a domain running with the feature turned off the
> @@ -1017,6 +1030,23 @@ virSecurityDACRestoreImageLabelSingle(virSecurityManagerPtr mgr,
> }
> }
>
> + /* This is not very clean. But so far we don't have NVMe
> + * storage pool backend so that its chownCallback would be
> + * called. And this place looks least offensive. */
> + if (src->type == VIR_STORAGE_TYPE_NVME) {
> + const virStorageSourceNVMeDef *nvme = src->nvme;
> + g_autofree char *vfioGroupDev = NULL;
> +
> + if (!(vfioGroupDev = virPCIDeviceAddressGetIOMMUGroupDev(&nvme->pciAddr)))
> + return -1;
> +
> + /* Ideally, we would check if there is not another PCI
> + * device within domain def that is in the same IOMMU
> + * group. But we're not doing that for hostdevs yet. */
> +
> + return virSecurityDACRestoreFileLabelInternal(mgr, NULL, vfioGroupDev, false);
> + }
> +
> return virSecurityDACRestoreFileLabelInternal(mgr, src, NULL, true);
> }
>
I think my suggested helpers will simplify selinux and apparmor a bit.
Not sure about DAC but I don't fully understand the chownCallback +
transaction stuff.
Looks mechanically sound AFAICT
Reviewed-by: Cole Robinson <crobinso@xxxxxxxxxx>
- Cole
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
