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

[David Gibson]

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.

> > 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.

> I've just one typo in the description of "host-serial" below. Appart from
> that:
> 
> Reviewed-by: Greg Kurz <address@hidden>
> 
> and
> 
> Tested-by: Greg Kurz <address@hidden>

Thanks.

> 
> >  hw/ppc/spapr.c         | 57 ++++++++++++++----------------------------
> >  include/hw/ppc/spapr.h |  1 +
> >  2 files changed, 20 insertions(+), 38 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 6c16d6cfaf..c46c6e2670 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1252,38 +1252,8 @@ static void *spapr_build_fdt(SpaprMachineState 
> > *spapr)
> >      _FDT(fdt_setprop_string(fdt, 0, "model", "IBM pSeries (emulated by 
> > qemu)"));
> >      _FDT(fdt_setprop_string(fdt, 0, "compatible", "qemu,pseries"));
> >  
> > -    /*
> > -     * Add info to guest to indentify which host is it being run on
> > -     * and what is the uuid of the guest
> > -     */
> > -    if (spapr->host_model && !g_str_equal(spapr->host_model, "none")) {
> > -        if (g_str_equal(spapr->host_model, "passthrough")) {
> > -            /* -M host-model=passthrough */
> > -            if (kvmppc_get_host_model(&buf)) {
> > -                _FDT(fdt_setprop_string(fdt, 0, "host-model", buf));
> > -                g_free(buf);
> > -            }
> > -        } else {
> > -            /* -M host-model=<user-string> */
> > -            _FDT(fdt_setprop_string(fdt, 0, "host-model", 
> > spapr->host_model));
> > -        }
> > -    }
> > -
> > -    if (spapr->host_serial && !g_str_equal(spapr->host_serial, "none")) {
> > -        if (g_str_equal(spapr->host_serial, "passthrough")) {
> > -            /* -M host-serial=passthrough */
> > -            if (kvmppc_get_host_serial(&buf)) {
> > -                _FDT(fdt_setprop_string(fdt, 0, "host-serial", buf));
> > -                g_free(buf);
> > -            }
> > -        } else {
> > -            /* -M host-serial=<user-string> */
> > -            _FDT(fdt_setprop_string(fdt, 0, "host-serial", 
> > spapr->host_serial));
> > -        }
> > -    }
> > -
> > +    /* Guest UUID & Name*/
> >      buf = qemu_uuid_unparse_strdup(&qemu_uuid);
> > -
> >      _FDT(fdt_setprop_string(fdt, 0, "vm,uuid", buf));
> >      if (qemu_uuid_set) {
> >          _FDT(fdt_setprop_string(fdt, 0, "system-id", buf));
> > @@ -1295,6 +1265,21 @@ static void *spapr_build_fdt(SpaprMachineState 
> > *spapr)
> >                                  qemu_get_vm_name()));
> >      }
> >  
> > +    /* Host Model & Serial Number */
> > +    if (spapr->host_model) {
> > +        _FDT(fdt_setprop_string(fdt, 0, "host-model", spapr->host_model));
> > +    } else if (smc->broken_host_serial_model && 
> > kvmppc_get_host_model(&buf)) {
> > +        _FDT(fdt_setprop_string(fdt, 0, "host-model", buf));
> > +        g_free(buf);
> > +    }
> > +
> > +    if (spapr->host_serial) {
> > +        _FDT(fdt_setprop_string(fdt, 0, "host-serial", 
> > spapr->host_serial));
> > +    } else if (smc->broken_host_serial_model && 
> > kvmppc_get_host_serial(&buf)) {
> > +        _FDT(fdt_setprop_string(fdt, 0, "host-serial", buf));
> > +        g_free(buf);
> > +    }
> > +
> >      _FDT(fdt_setprop_cell(fdt, 0, "#address-cells", 2));
> >      _FDT(fdt_setprop_cell(fdt, 0, "#size-cells", 2));
> >  
> > @@ -3352,12 +3337,12 @@ static void spapr_instance_init(Object *obj)
> >          spapr_get_host_model, spapr_set_host_model,
> >          &error_abort);
> >      object_property_set_description(obj, "host-model",
> > -        "Set host's model-id to use - none|passthrough|string", 
> > &error_abort);
> > +        "Host model to advertise in guest device tree", &error_abort);
> >      object_property_add_str(obj, "host-serial",
> >          spapr_get_host_serial, spapr_set_host_serial,
> >          &error_abort);
> >      object_property_set_description(obj, "host-serial",
> > -        "Set host's system-id to use - none|passthrough|string", 
> > &error_abort);
> > +        "Host serial number to advertise in guest deivce tree", 
> > &error_abort);
> 
> s/deivce/device
> 
> >  }
> >  
> >  static void spapr_machine_finalizefn(Object *obj)
> > @@ -4381,18 +4366,14 @@ DEFINE_SPAPR_MACHINE(4_0, "4.0", true);
> >  static void spapr_machine_3_1_class_options(MachineClass *mc)
> >  {
> >      SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> > -    static GlobalProperty compat[] = {
> > -        { TYPE_SPAPR_MACHINE, "host-model", "passthrough" },
> > -        { TYPE_SPAPR_MACHINE, "host-serial", "passthrough" },
> > -    };
> >  
> >      spapr_machine_4_0_class_options(mc);
> >      compat_props_add(mc->compat_props, hw_compat_3_1, hw_compat_3_1_len);
> > -    compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
> >  
> >      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");
> >      smc->update_dt_enabled = false;
> >      smc->dr_phb_enabled = false;
> > +    smc->broken_host_serial_model = true;
> >      smc->default_caps.caps[SPAPR_CAP_CFPC] = SPAPR_CAP_BROKEN;
> >      smc->default_caps.caps[SPAPR_CAP_SBBC] = SPAPR_CAP_BROKEN;
> >      smc->default_caps.caps[SPAPR_CAP_IBS] = SPAPR_CAP_BROKEN;
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 2b4c05a2ec..5ea8081041 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -118,6 +118,7 @@ struct SpaprMachineClass {
> >      bool use_ohci_by_default;  /* use USB-OHCI instead of XHCI */
> >      bool pre_2_10_has_unused_icps;
> >      bool legacy_irq_allocation;
> > +    bool broken_host_serial_model; /* present real host info to the guest 
> > */
> >  
> >      void (*phb_placement)(SpaprMachineState *spapr, uint32_t index,
> >                            uint64_t *buid, hwaddr *pio, 
> 

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