Re: [Qemu-devel] [PATCH for-4.0] spapr: Simplify handling of host-serial and host-model values

[David Gibson]

On Fri, Mar 29, 2019 at 02:34:37PM +0100, Greg Kurz wrote:
> On Fri, 29 Mar 2019 10:28:46 +1100
> David Gibson <address@hidden> wrote:
> 
> > On Thu, Mar 28, 2019 at 01:56:48PM +0100, Greg Kurz wrote:
> > > On Thu, 28 Mar 2019 15:40:25 +1100
> > > David Gibson <address@hidden> wrote:
> > >   
> > > > 27461d69a0f "ppc: add host-serial and host-model machine attributes
> > > > (CVE-2019-8934)" introduced 'host-serial' and 'host-model' machine
> > > > properties for spapr to explicitly control the values advertised to the
> > > > guest in device tree properties with the same names.
> > > > 
> > > > The previous behaviour on KVM was to unconditionally populate the device
> > > > tree with the real host serial number and model, which leaks possibly
> > > > sensitive information about the host to the guest.
> > > > 
> > > > To maintain compatibility for old machine types, we allowed those props
> > > > to be set to "passthrough" to take the value from the host as before.  
> > > > Or
> > > > they could be set to "none" to explicitly omit the device tree items.
> > > > 
> > > > Special casing specific values on what's otherwise a user supplied 
> > > > string
> > > > is very ugly.  So, this patch simplifies things by implementing the
> > > > backwards compatibility in a different way: we have a machine class flag
> > > > set for the older machines, and we only load the host values into the
> > > > device tree if A) they're not set by the user and B) we have that flag 
> > > > set.
> > > > 
> > > > This does mean that the "passthrough" functionality is no longer 
> > > > available
> > > > with the current machine type.  That's ok though: if a user or 
> > > > management
> > > > layer really wants the information passed through they can read it
> > > > themselves (OpenStack Nova already does something similar for x86).
> > > > 
> > > > It also means the user can't explicitly ask for the values to be omitted
> > > > on the old machine types.  I think that's an acceptable trade-off: if 
> > > > you
> > > > care enough about not leaking the host information you can either move 
> > > > to
> > > > the new machine type, or use a dummy value for the properties.
> > > > 
> > > > This also removes an odd inconsistency between running on a POWER and
> > > > non-POWER (or non-Linux) hosts: if the host information couldn't be read
> > > > from where we expect (in the host's device tree as exposed by Linux), 
> > > > we'd
> > > > fallback to omitting the guest device tree items.  
> > > 
> > > This is still the case, isn't it ? A pseries-3.1 machine on a POWER linux 
> > > host
> > > has the DT items but the same machine on any other setup doesn't have 
> > > them...
> > > or maybe^Wlikely I don't understand what you mean :)  
> > 
> > So, I was talking about the case of the new machine type there.  Which
> > admittedly probably wasn't terribly clear in context.
> > 
> 
> Ok... I guess I got confused by the "this also removes", like if you were
> saying "I've dropped the previous behavior for new machines and I've also
> fixed an inconsistency", whereas you just give an example of how broken
> the old behavior was.
> 
> > > > While we're there, improve some poorly worded comments, and the help 
> > > > text
> > > > for the properties.
> > > > 
> > > > Signed-off-by: David Gibson <address@hidden>
> > > > ---
> > > > 
> > > > I've (tentatively) put this into my ppc-for-4.0 tree already, which I
> > > > hope to push in the next few days.  I realize it's very late to make
> > > > such a cleanup in 4.0, however I'd like to clean up the interface
> > > > before it goes into a released version which we have to support for
> > > > ages.
> > > >   
> > > 
> > > Sure. So, I've tested on POWER, non-POWER, with KVM, with TCG. It works as
> > > expected. Just one remark: when running an old machine type under TCG on
> > > a POWER host, the DT items are populated with the host data if QEMU was
> > > built with KVM support, and missing if QEMU was built without KVM support.
> > > This makes me wonder if kvm_enabled() should be added somewhere in the
> > > picture... Anyway, this has always been here and could be addressed in
> > > some other patch.  
> > 
> > I don't see that there's much point.  Yes, the old behaviour is broken
> > and inconsistent, and the new machine type behaviour fixes that.  I
> > don't see much profit in tweaking the exact areas of inconsistency in
> > the old behaviour.
> > 
> 
> Yes you're right. BTW, if it is settled for good that new machine
> types won't look at the host DT, then maybe we should even get rid
> of kvmppc_get_host_serial() and kvmppc_get_host_model() and opencode
> them where they're being used ? So that nobody is tempted to use them
> in new code.

Yeah, I've considered it and may well do that at some point.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature