On Thu, Nov 10, 2011 at 9:57 AM, Markus Armbruster <armbru@xxxxxxxxxx> wrote: > Pekka Enberg <penberg@xxxxxxxxxx> writes: > >> Hi Anthony, >> >>> On 11/04/2011 03:38 AM, Pekka Enberg wrote: >>>> Hi Linus, >>>> >>>> Please consider pulling the latest KVM tool tree from: >>>> >>>> git://git.kernel.org/pub/scm/linux/kernel/git/penberg/linux.git >>>> kvmtool/for-linus >>>> >>> >>> [snip] >>> >>>> tools/kvm/virtio/net.c | 423 ++++++++ >>>> tools/kvm/virtio/pci.c | 319 ++++++ >>>> tools/kvm/virtio/rng.c | 185 ++++ >>>> 186 files changed, 19071 insertions(+), 179 deletions(-) >> >> On Wed, 9 Nov 2011, Anthony Liguori wrote: >>> So let's assume for a moment that a tool like this should live in >>> the kernel. What's disturbing about a PULL request like this is the >>> lack of reviewability of it and the lack of any real review from >>> people that understand what's going on in this code base. >>> >>> There are no Acked-by's by people that really understand what the >>> code is doing or that have domain expertise in filesystems and >>> networking. >>> >>> There are major functionality short comings in this code base, data >>> corruptors, and CVEs. I'm not saying that the kvm-tool developers >>> are bad developers, but the code is not at the appropriate quality >>> level for the kernel. It just looks pretty on the surface to people >>> that are used to the kernel coding style. >>> >>> To highlight a few of the issues: > [...] >>> 2) The qcow2 code is a filesystem implemented in userspace. Image >>> formats are file systems. It really should be reviewed by the >>> filesystem maintainers. There is absolutely no attempt made to >>> synchronize the metadata during write operations which means that >>> you do not have crash consistency of the meta data. >>> >>> If you experience a power failure or kvm-tool crashs, your image >>> will get corrupted. I highly doubt a file system would ever be >>> merged into Linux that was this naive about data integrity. >> >> The QCOW2 is lagging behind because we lost the main developer. It's >> forced as read-only for the issues you mention. If you think it's a >> merge blocker, we can drop it completely from the tree until the >> issues are sorted out. >> >> I personally don't see the issue of having it as a read-only filesystem. >> >>> 3) The block probing code replicates a well known CVE from three >>> years ago[1]. Using kvm-tool, a malicious guest could write the >>> qcow2 signature to the zero sector and use that to attack the host. >> >> We don't support QCOW2 snapshots so I don't see how the "arbitrary >> file" thing can happen. > > You don't need snapshots for the hole. > > Start with a clean read/write raw image. Probing declares it raw. > Guest writes QCOW signature to it, with a backing file of its choice. > > Restart with the same image. Probing declares it QCOW2. Guest can read > the backing file. Oops. Thats an excellent scenario why you'd want to have 'Secure KVM' with seccomp filters :) I'm actually not sure why KVM tool got QCOW support in the first place. You can have anything QCOW provides if you use btrfs (among several other FSs). -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
