[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 1/4] Provide support for the CUSE TPM
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH v5 1/4] Provide support for the CUSE TPM |
Date: |
Wed, 1 Mar 2017 20:30:19 +0200 |
On Wed, Mar 01, 2017 at 06:18:01PM +0000, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (address@hidden) wrote:
> > On Wed, Mar 01, 2017 at 06:06:02PM +0000, Dr. David Alan Gilbert wrote:
> > > * Michael S. Tsirkin (address@hidden) wrote:
> > > > On Wed, Mar 01, 2017 at 05:38:23PM +0000, Daniel P. Berrange wrote:
> > > > > On Wed, Mar 01, 2017 at 12:25:46PM -0500, Stefan Berger wrote:
> > > > > > On 03/01/2017 12:16 PM, Michael S. Tsirkin wrote:
> > > > > > > On Wed, Mar 01, 2017 at 12:12:34PM -0500, Stefan Berger wrote:
> > > > > > > > On 03/01/2017 12:02 PM, Michael S. Tsirkin wrote:
> > > > > > > > > On Wed, Mar 01, 2017 at 04:31:04PM +0000, Daniel P. Berrange
> > > > > > > > > wrote:
> > > > > > > > > > On Wed, Mar 01, 2017 at 06:22:45PM +0200, Michael S.
> > > > > > > > > > Tsirkin wrote:
> > > > > > > > > > > On Wed, Mar 01, 2017 at 09:50:38AM -0500, Stefan Berger
> > > > > > > > > > > wrote:
> > > > > > > > > > > > I had already proposed a linked-in version before I
> > > > > > > > > > > > went to the out-of-process
> > > > > > > > > > > > design. Anthony's concerns back then were related to
> > > > > > > > > > > > the code not being trusted
> > > > > > > > > > > > and a segfault in the code could bring down all of
> > > > > > > > > > > > QEMU. That we have test
> > > > > > > > > > > > suites running over it didn't work as an argument. Some
> > > > > > > > > > > > of the test suite are
> > > > > > > > > > > > private, though.
> > > > > > > > > > > Given how bad the alternative is maybe we should go back
> > > > > > > > > > > to that one.
> > > > > > > > > > > Same argument can be made for any device and we aren't
> > > > > > > > > > > making
> > > > > > > > > > > them out of process right now.
> > > > > > > > > > >
> > > > > > > > > > > IIMO it's less the in-process question (modularization
> > > > > > > > > > > of QEMU has been on the agenda since years and I don't
> > > > > > > > > > > think anyone is against it) it's more a code
> > > > > > > > > > > control/community question.
> > > > > > > > > > I rather disagree. Modularization of QEMU has seen few
> > > > > > > > > > results
> > > > > > > > > > because it is generally a hard problem to solve when you
> > > > > > > > > > have a
> > > > > > > > > > complex pre-existing codebase. I don't think code control
> > > > > > > > > > has
> > > > > > > > > > been a factor in this - as long as QEMU can clearly define
> > > > > > > > > > its
> > > > > > > > > > ABI/API between core & the modular pieces, it doesn't matter
> > > > > > > > > > who owns the module. We've seen this with vhost-user which
> > > > > > > > > > is
> > > > > > > > > > essentially outsourcing network device backend impls to a
> > > > > > > > > > 3rd
> > > > > > > > > > party project.
> > > > > > > > > And it was done precisely for community reasons. dpdk/VPP
> > > > > > > > > community is
> > > > > > > > > quite large and fell funded but they just can't all grok
> > > > > > > > > QEMU. They
> > > > > > > > > work for hardware vendors and do baremetal things. With the
> > > > > > > > > split we
> > > > > > > > > can focus on virtualization and they can focus on moving
> > > > > > > > > packets around.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > QEMU's defined the vhost-user ABI/API and delegated
> > > > > > > > > > impl to something else.
> > > > > > > > > The vhost ABI isn't easy to maintain at all though. So I
> > > > > > > > > would not
> > > > > > > > > commit to that lightly without a good reason.
> > > > > > > > >
> > > > > > > > > It will be way more painful if the ABI is dictated by a 3rd
> > > > > > > > > party
> > > > > > > > > library.
> > > > > > > > Who should define it?
> > > > > > > >
> > > > > > > No one. Put it in same source tree with QEMU and forget ABI
> > > > > > > stability
> > > > > > > issues.
> > > > > >
> > > > > > You mean put the code implementing TPM 1.2 and/or TPM 2 into the
> > > > > > QEMU tree?
> > > > > > These are multiple thousands of lines of code each and we'll break
> > > > > > them
> > > > > > apart into logical chunks and review them?
> > > > >
> > > > > No, lets not make that mistake again - we only just got rid of the
> > > > > libcacard smartcard library code from QEMU git.
> > > > >
> > > > > Regards,
> > > > > Daniel
> > > >
> > > > I don't mean that as an external library. As an integral part of QEMU
> > > > adhering to our coding style etc - why not?
> > > >
> > > > I don't know what are the other options. How is depending on an ABI
> > > > with a utility with no other users and not packaged by most distros
> > > > good? You are calling out to a CUSE device but who's reviewing that
> > > > code?
> > > >
> > > > vl.c weights in a 4500 lines of code. several thousand lines is
> > > > not small but not unmanageable.
> > >
> > >
> > > That's 4500 lines of fairly generic code; not like the TPM where the
> > > number
> > > of people who really understand the details of it is pretty slim.
> > >
> > > It's better on most counts to have it as a separate process.
> > >
> > > Dave
> >
> > Separate process we start and stop automatically I don't mind. A
> > separate tree with a distinct coding style where no one will ever even
> > look at it? Not so much.
>
> That code is used elsewhere anyway,
Who uses it? Who packages it? Fedora doesn't ...
> so asking them to change the coding style
> isn't very nice.
> Even if they change the coding style it doesn't mean you're suddenly going to
> understand how a TPM works in detail and be able to review it.
I did in the past but I didn't kept abreast of the recent developments.
> Anyway, having it in a separate process locked down by SELinux means that even
> if it does go horribly wrong it won't break qemu.
>
> Dave
Since qemu does blocking ioctls on it and doesn't validate results
too much it sure can break QEMU - anything from DOS to random
code execution. That's why we want to keep it in tree and
start it ourselves - I don't want CVEs claiming not validating
some parameter we get from it is a remote code execution.
It should be just a library that yes, we can keep out of
process for extra security but no, we can't just out random
stuff in there and never care.
> > > > Anyway, it all boils down to lack of reviewers. I know I am not merging
> > > > the current implementation because I could not figure out what do qemu
> > > > bits do without looking at the implementation. I don't want to jump
> > > > between so many trees and coding styles. bios/qemu/linux/dpdk are
> > > > painful enough to manage. If some other maintainer volunteers, or if
> > > > Peter wants to merge it directly from Stefan, I won't object.
> > > >
> > > > > --
> > > > > |: http://berrange.com -o-
> > > > > http://www.flickr.com/photos/dberrange/ :|
> > > > > |: http://libvirt.org -o-
> > > > > http://virt-manager.org :|
> > > > > |: http://entangle-photo.org -o-
> > > > > http://search.cpan.org/~danberr/ :|
> > > --
> > > Dr. David Alan Gilbert / address@hidden / Manchester, UK
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK
- Re: [Qemu-devel] [PATCH v5 1/4] Provide support for the CUSE TPM, (continued)
- Re: [Qemu-devel] [PATCH v5 1/4] Provide support for the CUSE TPM, Stefan Berger, 2017/03/01
- Re: [Qemu-devel] [PATCH v5 1/4] Provide support for the CUSE TPM, Daniel P. Berrange, 2017/03/01
- Re: [Qemu-devel] [PATCH v5 1/4] Provide support for the CUSE TPM, Michael S. Tsirkin, 2017/03/01
- Re: [Qemu-devel] [PATCH v5 1/4] Provide support for the CUSE TPM, Dr. David Alan Gilbert, 2017/03/01
- Re: [Qemu-devel] [PATCH v5 1/4] Provide support for the CUSE TPM, Michael S. Tsirkin, 2017/03/01
- Re: [Qemu-devel] [PATCH v5 1/4] Provide support for the CUSE TPM, Dr. David Alan Gilbert, 2017/03/01
- Re: [Qemu-devel] [PATCH v5 1/4] Provide support for the CUSE TPM,
Michael S. Tsirkin <=
- Re: [Qemu-devel] [PATCH v5 1/4] Provide support for the CUSE TPM, Stefan Berger, 2017/03/01
- Re: [Qemu-devel] [PATCH v5 1/4] Provide support for the CUSE TPM, Michael S. Tsirkin, 2017/03/01
- Re: [Qemu-devel] [PATCH v5 1/4] Provide support for the CUSE TPM, Michael S. Tsirkin, 2017/03/01
- Re: [Qemu-devel] [PATCH v5 1/4] Provide support for the CUSE TPM, Daniel P. Berrange, 2017/03/01
- Re: [Qemu-devel] [PATCH v5 1/4] Provide support for the CUSE TPM, Michael S. Tsirkin, 2017/03/01
- Re: [Qemu-devel] [PATCH v5 1/4] Provide support for the CUSE TPM, Marc-André Lureau, 2017/03/01
- Re: [Qemu-devel] [PATCH v5 1/4] Provide support for the CUSE TPM, Daniel P. Berrange, 2017/03/01
- Re: [Qemu-devel] [PATCH v5 1/4] Provide support for the CUSE TPM, Marc-André Lureau, 2017/03/01
- Re: [Qemu-devel] [PATCH v5 1/4] Provide support for the CUSE TPM, Michael S. Tsirkin, 2017/03/01
- Re: [Qemu-devel] [PATCH v5 1/4] Provide support for the CUSE TPM, Daniel P. Berrange, 2017/03/01