[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-4.0] spapr: Simplify handling of host-serial
From: |
David Gibson |
Subject: |
Re: [Qemu-devel] [PATCH for-4.0] spapr: Simplify handling of host-serial and host-model values |
Date: |
Fri, 29 Mar 2019 10:28:46 +1100 |
User-agent: |
Mutt/1.11.3 (2019-02-01) |
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