[libvirt] Investigation and possible fix of 1361592 - apparmor profiles do not include backing files

Hi, I've looked into why apparmor profiles do not contain exceptions for backing files of images which later leads to permission errors due to apparmor containment. As of newest libvirt git master, only the first level backing image is included, the subsequent images are omitted. Below is my investigation of why this issue happens. It is based on libvirt git master as of 2018-08-15 (e3e48d7cb82d58). The possible fix is easy, but I don't have the background how to write tests for it, so it's best that someone who knows the project well completes the fix In my case I have the following file hierarchy: - image-master.qcow2 (has backing file image-backing-1.qcow2) - image-backing-1.qcow2 (has backing file image-backing-2.qcow2 - image-backing-2.qcow2 The apparmor profiles are created by the virt-aa-helper tool. The get_files function (src/security/virt-aa-helper.c:949) creates a list of files that need to be accessed by the virtual machine. The call to virDomainDiskDefForeachPath() is responsible for creating the list of files required to access each disk given the disk metadata. The disk->src argument contains the source file. The expected file hierarchy would be this: disk->src->path == path/to/image-master.qcow2 disk->src->backingStore->path == path/to/image-backing-1.qcow2 disk->src->backingStore->backingStore->path == path/to/image-backing-2.qcow2 Unfortunately only the first two levels are present and disk->src->backingStore->backingStore points to a dummy object. The backing store details are filled in virStorageFileGetMetadata() call. It calls into virStorageFileGetMetadataRecurse (src/util/virstoragefile.c:4789) which will collect metadata for a single image and recurse into itself for backing files. For us, the following part of virStorageFileGetMetadataRecurse is important (simplified for brevity): ``` virStorageFileGetMetadataInternal(src, ..., &backingFormat); if (src->backingStoreRaw) { backingStore = ... if (backingFormat == VIR_STORAGE_FILE_AUTO) backingStore->format = VIR_STORAGE_FILE_RAW; [1] else if (backingFormat == VIR_STORAGE_FILE_AUTO_SAFE) backingStore->format = VIR_STORAGE_FILE_AUTO; else backingStore->format = backingFormat; virStorageFileGetMetadataRecurse(backingStore, ...) [2] } ``` The crux of the issue seems that the call to virStorageFileGetMetadataInternal() for the image-master.qcow2 image will set the `backingFormat` return value to VIR_STORAGE_FILE_AUTO. The code responsible is in qcowXGetBackingStore (src/util/virstoragefile.c:491) called via `fileTypeInfo[meta->format].getBackingStore(...)` at src/util/virstoragefile.c:1042. It backingFormat is then downgraded to VIR_STORAGE_FILE_RAW at src/util/virstoragefile.c:4854 ([1] above). This causes the next recurse call to virStorageFileGetMetadataRecurse() ([2] above) to not investigate the backing images at all in virStorageFileGetMetadataInternal() as fileTypeInfo[VIR_STORAGE_FILE_RAW].getBackingStore will be NULL. The possible solution is to return VIR_STORAGE_FILE_AUTO_SAFE from qcowXGetBackingStore. It probably does not make much sense to prevent probing of qcow2 backing images as we probe the first level of backing images anyway, so that return value only affect second and subsequent levels of backing images. Looking into the implementation of qcowXGetBackingStore it also does not seem that it performs any operations that are unsafe by design. Currently VIR_STORAGE_FILE_AUTO is used only in qedGetBackingStore and it does not seem that probing of qcow images is unsafe enough What do the developers think about this? Could someone complete the fix with tests or advise me about the testing frameworks if there's not enough time? Thanks a lot! Povilas