Re: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances

On Wed, Feb 19, 2020 at 13:12:53 -0600, Eric Blake wrote: Adding Daniel as he objected to qemu-img. [...] I raised the use of qemu-img to Daniel and he disagreed with use of qemu-img in libvirt for doing the probing on multiple reasons: - qemu-img instantiates many data structures relevant to the format so it has a huge attack surface - performance of spawning extra processes would be way worse While at least from the point of view of VM startup both can be challenged this adds a complete new orthogonal dimension to the problem I'm attempting to fix. I'll reiterate the historical state of the problem because I think it's important: Pre-blockdev: - we internally assumed that if the image format of an backing image was not present in the overlay it is 'raw' - this influenced security labelling but not actually how qemu viewed or probed the file. If it was qcow2 probed as qcow2 qemu opened it as qcow2 possibly even including the backing file if selinux or other mechanism didn't prevent it. post-blockdev: - the assumption of 'raw' would now be expressed into the qemu configuration. This assumption turned into data corruption since we no longer allowed qemu to probe the format and forced it as raw. - fix was to always require the format to be recorded in the overlay - this made users unhappy who neglected to record the format into the overlay when creating it manually Now since qemu didn't discourage the creation of overlays without format there still are many users which will inevitably hit this problem when used with libvirt. My proposal tries to mitigate the regressions in behaviour in the valid and secure use cases. (If the image whose format we detect doesn't have a backing image) This comes at a trade-off though. As Eric pointed out, if the format probed by libvirt's internal code disagrees with qemu's format we are getting into the image corruption region. As a mitigation to the above I suggested using qemu-img to probe but that's a complex change and as mentioned above not really welcome upstream. Now this adds an interresting dimension to this problem. If libvirt forces the users to specify the image format, and the users don't know it they will probe. So we are basically making this a problem of somebody else. [2] As you can see in that patch, it uses 'qemu-img' anyways and also additionally actually allows the chain to continue deeper! [3] A partial relief to the image detection problem is that qemu would refuse to start if an non-qcow2 image is used in qcow2, thus we really only run into problems if qcow2 is mis-detected as raw. This boils down to whether we want to accept some possibility of image corruption in trade for avoiding regression of behaviour in the secure cases as well as management apps and users not having to re-invent when probing an image is actually safe. Finally I think we should either decide to fix it in this release, or stick with the error message forever. Fixing it later will not make much sense as many users already fixed their scripts and we'd just add back the trade-off of possible image corruption. Peter [1] If e.g. the security subsystem of the host didn't forbid the use of the backing file such a qcow2 qemu would happily open it. [2] https://www.redhat.com/archives/libguestfs/2020-February/msg00013.html [3] As implemented in [2] the backing image is not checked whether it has a backing file or not but the format is probed, which way result into accessing the backing chain of the probed image. Prior to this detection, it would be prevented by sVirt or alternatively also by libvit itself in -blockdev mode when this patch would be accepted.