Message-ID: <4EBB4A21.20707@codemonkey.ws>
Date:	Wed, 09 Nov 2011 21:50:57 -0600
From:	Anthony Liguori <anthony@...emonkey.ws>
To:	Pekka Enberg <penberg@...helsinki.fi>
CC:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Avi Kivity <avi@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Ingo Molnar <mingo@...e.hu>, linux-kernel@...r.kernel.org,
	kvm@...r.kernel.org, Christoph Hellwig <hch@....de>
Subject: Re: [RFC/GIT PULL] Linux KVM tool for v3.2

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(-)

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:

1) The RTC emulation is limited to emulating CMOS and only the few fields used 
to store the date and time.  If code is added to arch/x86 that tries to make use 
of a CMOS field for something useful, kvm-tool is going to fall over.

None of the register A/B/C logic is implemented and none of the timer logic is 
implemented.  I imagine this requires kernel command line hackery to keep the 
kernel from throwing up.

If a kernel change that works on bare metal but breaks kvm-tool because kvm-tool 
is incomplete is committed, is that a regression that requires reverting the 
change in arch/x86?

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.

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.

I found these three issues in the course of about 30 seconds of looking through 
the kvm-tool code.  I'm sure if other people with expertise in these areas 
looked through the code, they would find a lot more issues.  I'm sure I could 
find many, many more issues.

This is really the problem with the tools/kvm approach.  It circumvents the 
normal review process in the kernel because the kernel maintainer structure is 
not equipped to properly review userspace code in tools.  This is a tool with 
data integrity and security implications.  It is not a pretty printing routine 
or a test case.

While I think it's a neat and potentially useful project, I think long before we 
get to the point where we discuss merging it into the kernel, the code quality 
has to improve considerably.

[1] http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2008-2004

Regards,

Anthony Liguori
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists