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.

While we're there, improve some poorly worded comments, and the help text
for the properties.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---

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.

 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);
 }
 
 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, 
-- 
2.20.1

On Thu, Mar 28, 2019 at 03:40:25PM +1100, David Gibson 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.
> 
> While we're there, improve some poorly worded comments, and the help text
> for the properties.

So IIUC, the two properties now only accept an opaque string which
will be exposes as-is in the guest fields. Old machine types, only,
will do passthrough of the host values (if not overriden by the
properties) & there's no way to request this for new machine types

> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> 
> 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.

Indeed, we must clean it before release if we want this, otherwise
it is an incompatible change.

> 
>  hw/ppc/spapr.c         | 57 ++++++++++++++----------------------------
>  include/hw/ppc/spapr.h |  1 +
>  2 files changed, 20 insertions(+), 38 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Thu, Mar 28, 2019 at 09:55:24AM +0000, Daniel P. Berrangé wrote:
> On Thu, Mar 28, 2019 at 03:40:25PM +1100, David Gibson 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.
> > 
> > While we're there, improve some poorly worded comments, and the help text
> > for the properties.
> 
> So IIUC, the two properties now only accept an opaque string which
> will be exposes as-is in the guest fields. Old machine types, only,
> will do passthrough of the host values (if not overriden by the
> properties) & there's no way to request this for new machine types

Correct.

> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > 
> > 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.
> 
> Indeed, we must clean it before release if we want this, otherwise
> it is an incompatible change.
> 
> > 
> >  hw/ppc/spapr.c         | 57 ++++++++++++++----------------------------
> >  include/hw/ppc/spapr.h |  1 +
> >  2 files changed, 20 insertions(+), 38 deletions(-)
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> 
> Regards,
> Daniel

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

On Thu, 28 Mar 2019 15:40:25 +1100
David Gibson <david@gibson.dropbear.id.au> 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 :)

> While we're there, improve some poorly worded comments, and the help text
> for the properties.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> 
> 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've just one typo in the description of "host-serial" below. Appart from
that:

Reviewed-by: Greg Kurz <groug@kaod.org>

and

Tested-by: Greg Kurz <groug@kaod.org>

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

On Thu, Mar 28, 2019 at 01:56:48PM +0100, Greg Kurz wrote:
> On Thu, 28 Mar 2019 15:40:25 +1100
> David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
> > ---
> > 
> > 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 <groug@kaod.org>
> 
> and
> 
> Tested-by: Greg Kurz <groug@kaod.org>

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

On Fri, 29 Mar 2019 10:28:46 +1100
David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
> > > ---
> > > 
> > > 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.

> > I've just one typo in the description of "host-serial" below. Appart from
> > that:
> > 
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> > 
> > and
> > 
> > Tested-by: Greg Kurz <groug@kaod.org>  
> 
> 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,   
> >   
> 

On Fri, Mar 29, 2019 at 02:34:37PM +0100, Greg Kurz wrote:
> On Fri, 29 Mar 2019 10:28:46 +1100
> David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
> > > > ---
> > > > 
> > > > 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