Re: [RFC/GIT PULL] Linux KVM tool for v3.2

[Sasha Levin]

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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/