Re: [Qemu-devel] [Qemu-ppc] [PATCH v7 0/2] spapr-rtas: add ibm, get-vpd RTAS interface

[Greg Kurz]

On Mon, 8 Apr 2019 14:21:50 +1000
David Gibson <address@hidden> wrote:

> On Fri, Mar 29, 2019 at 01:29:51PM +0100, Greg Kurz wrote:
> > On Thu, 28 Mar 2019 15:39:45 -0300
> > "Maxiwell S. Garcia" <address@hidden> wrote:
> >   
> > > Hi,
> > > 
> > > On Thu, Mar 28, 2019 at 02:21:51PM +0100, Greg Kurz wrote:  
> > > > On Wed, 27 Mar 2019 17:41:00 -0300
> > > > "Maxiwell S. Garcia" <address@hidden> wrote:
> > > >     
> > > > > Here are two patches to add a handler for ibm,get-vpd RTAS calls.
> > > > > This RTAS exposes host information in case of set QEMU options
> > > > > 'host-serial' and 'host-model' as 'passthrough'.
> > > > > 
> > > > > The patch 1 creates helper functions to get valid 'host-serial'
> > > > > and 'host-model' parameters, guided by QEMU command line. These
> > > > > parameters are useful to build the guest device tree and to return
> > > > > get-vpd RTAS calls. The patch 2 adds the ibm,get-vpd itself.
> > > > > 
> > > > > Update v7:
> > > > > * rtas_get_vpd_fields as a static array in spapr machine state
> > > > > 
> > > > > Maxiwell S. Garcia (2):
> > > > >   spapr: helper functions to get valid host fields
> > > > >   spapr-rtas: add ibm,get-vpd RTAS interface
> > > > > 
> > > > >  hw/ppc/spapr.c         | 48 +++++++++++----------
> > > > >  hw/ppc/spapr_rtas.c    | 96 
> > > > > ++++++++++++++++++++++++++++++++++++++++++
> > > > >  include/hw/ppc/spapr.h | 14 +++++-
> > > > >  3 files changed, 135 insertions(+), 23 deletions(-)
> > > > >     
> > > > 
> > > > Hi Maxiwell,
> > > > 
> > > > David sent a patch to rework how the host data is exposed to the guest.
> > > > Especially, the special casing of the "none" and "passthrough" strings
> > > > is no more... I'm afraid you'll have to rework your patches accordingly:
> > > > code+changelog in patch 1 and at least changelog in patch 2.
> > > > 
> > > > Cheers,    
> > > 
> > > IIUC, the 'ibm,get-vpd' RTAS should return information about the
> > > platform/cabinet. Thus, it's not necessary to add new nodes in the guest
> > > device tree to export information like that.  
> > 
> > I agree that these "host-model" and "host-serial" props, which aren't
> > described anywhere and not used by either the linux kernel or the
> > powerpc-utils, look like a QEMU-specific poor man's version of VPD.
> > 
> > Not quite sure why they were even created since this is the purpose
> > of "system-id" and "model" as explained in PAPR, and supposedly
> > exposed in /proc/ppc64/lparcfg according to the LPARCFG(5) manual
> > page:  
> 
> Yeah, I'm not sure why they were created either.  I rather suspect
> nothing much is using them, and I'd kind of like to just kill them.
> But Daniel Berrange (and maybe others) are paranoid about this
> breaking things.
> 

Speaking of that. The "host-model"/"host-serial" fix is associated to a
CVE which affects QEMU versions currently shipped by downstream vendors.
Isn't a good enough reason to break things in existing unsecure setups ?
Should we add this patch to Mike's patch round-up for stable 3.0.1 (and
therefore break something that used to _work_ with 3.0.0) ?

> > 
> >        serial_number
> >        The serial number of the physical system in which the partition 
> > resides
> > 
> >        system_type
> >        The  machine,type-model  of  the physical system in which the 
> > partition
> >        resides
> > 
> > This is indeed what we get in a PowerVM LPAR running on a tuleta system:
> > 
> > address@hidden ~]# head -3 /proc/ppc64/lparcfg 
> > lparcfg 1.9
> > serial_number=IBM,032116A9A
> > system_type=IBM,8247-22L
> > 
> > address@hidden ~]# echo $(cat /proc/device-tree/system-id)
> > IBM,032116A9A
> > address@hidden ~]# echo $(cat /proc/device-tree/model)
> > IBM,8247-22L
> > 
> > But QEMU generates a hard coded "IBM pSeries (emulated by qemu)" model,
> > which is clearly not PAPR compliant according to this requirement:
> > 
> >     R1–12.2–13. There must be a property, “model”, under the root node
> >     in the format, “<vendor>,xxxx-yyy”, where <vendor> is replaced by
> >     one to five letters representing the stock symbol of the company
> >     (for example, for IBM: “IBM,xxxx-yyy”), and where xxxx-yyy is
> >     derived from the VPD TM field (see Table 160‚ “LoPAPR VPD Fields‚”
> >     on page 343) of the first or ‘marked’ processor enclosure.
> > 
> > And worse, it mixes "vm,uuid" which is QEMU specific concept to uniquely
> > identify guests, with "system-id" which is about the host :-\  
> 
> Ugh, right.  So, it's been this way for years, so it's clearly not
> breaking things in practice.  Given that, it might be best to leave
> it, even though it violates PAPR technically.
> 
> Frankly, providing information about the *guest* virtual model and
> "serial number" =~= UUID makes more sense than providing information
> about the host that might be security sensitive.
> 

IIUC, the rationale of exposing to the guest something that uniquely
identifies the host is needed by some inventory or license validation
softwares [1]. So I'd say we need both a guest UUID, that doesn't change
during the VM lifetime, and host UUID that changes when migrating to
another host.

The fact that "system-id" == "vm,uuid" in the current code didn't
break anything would rather indicate that there are no inventory
or license validation softwares users yet in the KVM+PAPR+linux
ecosystem...

[1] https://bugs.launchpad.net/nova/+bug/1337349/comments/6

> > All of this is very confusing and need to be sorted out before building
> > anything on top of it. Especially since "model" and "system-id" are
> > supposed to derive from VPD IIUC.
> > 
> > I guess that we should first decide what we really want to expose
> > in "system-id" and "model": what we have now ? the same as in
> > "host-serial" and "host-model", ie. user configurable ? Must we
> > stay compatible with existing setups ? In any case, I'm afraid that
> > we have to diverge from PAPR somehow, since it obviously doesn't
> > care about the security concerns that motivated recent changes
> > for "host-serial" and "host-model"...  
> 
> We absolutely should not expose the real host information without the
> user explicitly requesting it.
> 

I agree, but it seems that we may need something in between: an ID that
uniquely identifies a given system but that doesn't reveal security
sensitive information.

Looking again at PAPR, nothing much is said on the format of "system-id"
or VPD SN. Only "model" is expected to follow some formatting rules:
<vendor>,xxxx-yyy.

For "system-id", this could be addressed pretty much like in OpenStack
with systemd's /etc/machine-id. For "model", I'm not sure...

> > > Since it's a POWER specific
> > > functionality, may 'ibm,get-vpd' export host information if the
> > > guest instance allows it? Or is it better return only the 'host-serial'
> > > and 'host-model' content, like in the patch "spapr: Simplify handling
> > > of host-serial and host-model values"?
> > >   
> > 
> > I've spent some time reading PAPR on this topic and I'm not sure that
> > "ibm,get-vpd" is the way to go for what you were trying to achieve
> > initially (ie, obtain up-to-date host model and serial after migration).
> > 
> > Have you looked at RTAS "ibm,update-properties" ?
> > 
> >     7.4.8 ibm,update-properties RTAS Call
> > 
> >     This RTAS call is used to report updates to the properties changed
> >     due to a massive platform reconfiguration such as when the partition
> >     is migrated between machines.  
> 
> Yeah.. the thing here is that partition migration kind of means
> something different in PAPR than it does in the qemu world.  It uses a
> guest-aware model of migration (which frankly I think is broken
> verging on unworkable).  qemu and uses a guest-(mostly)-unaware
> migration model.
> 

Yeah...

> > This explicitly covers updates to "system-id" and "model". Maybe it is
> > time to do as Ben was suggesting a long time ago when "host-serial"
> > and "host-model" were introduced [1]:
> > 
> >     On Tue, 2014-07-08 at 12:49 +0200, Alexander Graf wrote:  
> >     > Please be aware that all of the above is bogus when you start
> >     > thinking 
> >     > about live migration.  
> > 
> >     What's probably where we need to start thinking about implementing
> >     migration according to PAPR :-)
> > 
> >     IE. With pre and post-migration notifications to the guest including
> >     device-tree updates.
> > 
> > 
> > [1] https://patchwork.ozlabs.org/patch/367792/#813547
> >   
>

pgpb47x9c1Cas.pgp
Description: OpenPGP digital signature