On Thu, Mar 28, 2019 at 01:56:48PM +0100, Greg Kurz wrote: > On Thu, 28 Mar 2019 15:40:25 +1100 > David Gibson <da...@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 <da...@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 <gr...@kaod.org> > > and > > Tested-by: Greg Kurz <gr...@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
signature.asc
Description: PGP signature