Hi Michal, Thank you for your super quick response - including patches, no less!
Several months ago, I ran into issue #135 which says that Qemu under AppArmor can't access LVM volume disks.
The problem here is, that AppArmor driver in libvirt generates the profile in qemuSecurityGenLabel() phase. Long story short, starting up a libvirt domain is broken down into several steps. One of them is qemuProcessPrepareDomain() where domain XML is filled with (parts of) live data. In this specific case, disk source is translated (from storage pool/vol to a filepath). But it's not just that, various other paths are generated (e.g. socket paths, render nodes for graphics, etc.).
Thanks for this context. Yes, I was unaware of that situation and completely focussed at the volume paths. Taking the extra paths into account complicates the matter.
Then, the next step is qemuProcessPrepareHost() where those files from previous step are created, their seclabels are (possibly) set. This has a caveat though.
Initially, libvirt cared about SELinux only (leave traditional uid/gid perms (we call them DAC) aside for a brief moment). Here. an unique seclabel is generated in the domain prepare phase, so that any helper process that's used to create files in host prepare phase can run under that label (and thus be restricted).
AppArmor does not work this way. So when an aforementioned helper process wants to run (say /usr/bin/swtpm_setup) it already needs profile to be defined. As "obvious" workaround, AppArmor generates the profile in seclabel generation phase. But by that time path are not populated!
I saw your patches. From the patches I concluded that the security policy is strictly applicable to Qemu (apparently). That greatly simplifies the solution space, but with your proposed patches, I see that the learning curve would have been too steep for me to be able to come up with a solution this simple.
IMO, quick and dirty fix would be to generate seclabels at the end of qemuProcessPrepareDomain() instead of beginning (see below). The only downside of this is that seclabels won't be available upfront, e.g. if we want to copy them into a device specific seclabel (we do NOT do that though). diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c index a53bb40783..00b9a732f3 100644 --- i/src/qemu/qemu_process.c +++ w/src/qemu/qemu_process.c @@ -7024,15 +7024,6 @@ qemuProcessPrepareDomain(virQEMUDriver *driver, return -1;
if (!(flags & VIR_QEMU_PROCESS_START_PRETEND)) { - /* If you are using a SecurityDriver with dynamic labelling, - then generate a security label for isolation */ - VIR_DEBUG("Generating domain security label (if required)"); - if (qemuSecurityGenLabel(driver->securityManager, vm->def) < 0) { - virDomainAuditSecurityLabel(vm, false); - return -1; - } - virDomainAuditSecurityLabel(vm, true); - if (qemuProcessPrepareDomainNUMAPlacement(vm) < 0) return -1; } @@ -7154,6 +7145,17 @@ qemuProcessPrepareDomain(virQEMUDriver *driver, } }
+ /* Keep this as the very last step because AppArmor already generates + * profile at this point. IOW, we need all the paths filled in. */ + if (!(flags & VIR_QEMU_PROCESS_START_PRETEND)) { + VIR_DEBUG("Generating domain security label (if required)"); + if (qemuSecurityGenLabel(driver->securityManager, vm->def) < 0) { + virDomainAuditSecurityLabel(vm, false); + return -1; + } + virDomainAuditSecurityLabel(vm, true); + } + return 0; }
Another alternative is to move profile generation into its separate step. So then we'd have:
qemuProcessPrepareDomain(): 1) gen seclabel /* here only SELinux/DAC drivers would do something useful. For AppArmor it'd be a NOP. */ 2) generate all the paths 3) write profile /* only AppArmor would act upon, for SELinux and AppArmor it'd be nop. */
Let me see if that'd would fix the issue.
From the fact that you submitted the patches, I think I understand the change above didn't work. Is there anything I can do to help this patch set? -- Bye, Erik. http://efficito.com -- Hosted accounting and ERP. Robust and Flexible. No vendor lock-in.