Re: [Qemu-devel] [PATCH] ppc: add host-serial and host-model machine attributes

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 9173 bytes --]

On Sat, Feb 02, 2019 at 12:23:58AM +0530, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> On ppc hosts, hypervisor shares following system attributes
> 
>   - /proc/device-tree/system-id
>   - /proc/device-tree/model
> 
> with a guest. This could lead to information leakage and misuse.[*]
> Add machine attributes to control such system information exposure
> to a guest.
> 
> [*] https://wiki.openstack.org/wiki/OSSN/OSSN-0028
> 
> Reported-by: Daniel P. Berrangé <berrange@redhat.com>
> Fix-suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>

Hm.  This seems like it might be overkill.  I mean, obviously we need
to not leak that host information, but it's not clear we really need
these properties at all.  They're not specified in PAPR (contrary to
my previous guess) and it's not clear what actually uses them.

I'm wondering if we can just ditch them entirely, or at least make
them default to not present without regard to machine version.

Yes, that's technically a compatibility breaking change, but it's hard
to see anything that actually relied on these as not being broken
already, so I think that's actually a fair trade off for the security
improvement here.

> ---
>  hw/core/machine.c   | 46 +++++++++++++++++++++++++++++++++++++++++++++
>  hw/ppc/spapr.c      | 27 ++++++++++++++++++++++++--
>  include/hw/boards.h |  2 ++
>  qemu-options.hx     | 10 +++++++++-
>  util/qemu-config.c  |  8 ++++++++
>  5 files changed, 90 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 2629515363..2d5a52476a 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -476,6 +476,38 @@ static void machine_set_memory_encryption(Object *obj, const char *value,
>      ms->memory_encryption = g_strdup(value);
>  }
>  
> +static char *machine_get_host_serial(Object *obj, Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +
> +    return g_strdup(ms->host_serial);
> +}
> +
> +static void machine_set_host_serial(Object *obj, const char *value,
> +                                    Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +
> +    g_free(ms->host_serial);
> +    ms->host_serial = g_strdup(value);
> +}
> +
> +static char *machine_get_host_model(Object *obj, Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +
> +    return g_strdup(ms->host_model);
> +}
> +
> +static void machine_set_host_model(Object *obj, const char *value,
> +                                   Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +
> +    g_free(ms->host_model);
> +    ms->host_model = g_strdup(value);
> +}
> +
>  void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type)
>  {
>      strList *item = g_new0(strList, 1);
> @@ -760,6 +792,18 @@ static void machine_class_init(ObjectClass *oc, void *data)
>          &error_abort);
>      object_class_property_set_description(oc, "memory-encryption",
>          "Set memory encryption object to use", &error_abort);
> +
> +    object_class_property_add_str(oc, "host-serial",
> +        machine_get_host_serial, machine_set_host_serial,
> +        &error_abort);
> +    object_class_property_set_description(oc, "host-serial",
> +        "Set host's system-id to use", &error_abort);
> +
> +    object_class_property_add_str(oc, "host-model",
> +        machine_get_host_model, machine_set_host_model,
> +        &error_abort);
> +    object_class_property_set_description(oc, "host-model",
> +        "Set host's model-id to use", &error_abort);
>  }
>  
>  static void machine_class_base_init(ObjectClass *oc, void *data)
> @@ -785,6 +829,8 @@ static void machine_initfn(Object *obj)
>      ms->dump_guest_core = true;
>      ms->mem_merge = true;
>      ms->enable_graphics = true;
> +    ms->host_serial = NULL;
> +    ms->host_model = NULL;
>  
>      /* Register notifier when init is done for sysbus sanity checks */
>      ms->sysbus_notifier.notify = machine_init_notify;
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 0942f35bf8..b497fe1701 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1249,11 +1249,34 @@ static void *spapr_build_fdt(sPAPRMachineState *spapr,
>       * Add info to guest to indentify which host is it being run on
>       * and what is the uuid of the guest
>       */
> -    if (kvmppc_get_host_model(&buf)) {
> +    if (machine->host_model && !strcmp(machine->host_model, "none")) {
> +        /* -M host-model=none = do not set host-model */
> +    } else if (machine->host_model
> +        && !strcmp(machine->host_model, "passthrough")) {
> +        /* -M host-model=passthrough */
> +        _FDT(fdt_setprop_string(fdt, 0, "host-model", buf));
> +        g_free(buf);
> +    } else if (machine->host_model) {
> +        /* -M host-model=<user-string> */
> +        _FDT(fdt_setprop_string(fdt, 0, "host-model", machine->host_model));
> +    } else if (kvmppc_get_host_model(&buf)) {
> +        /* -M host-model=xxx attribute not supplied */
>          _FDT(fdt_setprop_string(fdt, 0, "host-model", buf));
>          g_free(buf);
>      }
> -    if (kvmppc_get_host_serial(&buf)) {
> +
> +    if (machine->host_serial && !strcmp(machine->host_serial, "none")) {
> +        /* -M host-serial=none = do not set host-serial */
> +    } else if (machine->host_serial
> +        && !strcmp(machine->host_serial, "passthrough")) {
> +        /* -M host-serial=passthrough */
> +        _FDT(fdt_setprop_string(fdt, 0, "host-serial", buf));
> +        g_free(buf);
> +    } else if (machine->host_serial) {
> +        /* -M host-serial=<user-string> */
> +        _FDT(fdt_setprop_string(fdt, 0, "host-serial", machine->host_serial));
> +    } else if (kvmppc_get_host_serial(&buf)) {
> +        /* -M host-serial=xxx attribute not supplied */
>          _FDT(fdt_setprop_string(fdt, 0, "host-serial", buf));
>          g_free(buf);
>      }
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 02f114085f..3e63dc4501 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -257,6 +257,8 @@ struct MachineState {
>      bool enforce_config_section;
>      bool enable_graphics;
>      char *memory_encryption;
> +    char *host_serial;
> +    char *host_model;
>      DeviceMemoryState *device_memory;
>  
>      ram_addr_t ram_size;
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 521511ec13..67ac1a9959 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -43,7 +43,9 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
>      "                suppress-vmdesc=on|off disables self-describing migration (default=off)\n"
>      "                nvdimm=on|off controls NVDIMM support (default=off)\n"
>      "                enforce-config-section=on|off enforce configuration section migration (default=off)\n"
> -    "                memory-encryption=@var{} memory encryption object to use (default=none)\n",
> +    "                memory-encryption=@var{} memory encryption object to use (default=none)\n"
> +    "                host-serial=none|passthrough|string controls host systemd-id (default=passthrough)\n"
> +    "                host-model=none|passthrough|string controls host model-id (default=passthrough)\n",
>      QEMU_ARCH_ALL)
>  STEXI
>  @item -machine [type=]@var{name}[,prop=@var{value}[,...]]
> @@ -103,6 +105,12 @@ NOTE: this parameter is deprecated. Please use @option{-global}
>  @option{migration.send-configuration}=@var{on|off} instead.
>  @item memory-encryption=@var{}
>  Memory encryption object to use. The default is none.
> +@item host-serial=none|passthrough|string
> +Pass 'system-id' parameter from host's device-tree to a guest. A user may
> +disable it with 'none' or define a custom string for a guest.
> +@item host-model=none|passthrough|string
> +Pass 'model' paramter from host's device-tree to a guest. A user may disable
> +it with 'none' or define a custom string for a guest.
>  @end table
>  ETEXI
>  
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index 9d2e278e29..86483ded34 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -238,6 +238,14 @@ static QemuOptsList machine_opts = {
>              .help = "Up to 8 chars in set of [A-Za-z0-9. ](lower case chars"
>                      " converted to upper case) to pass to machine"
>                      " loader, boot manager, and guest kernel",
> +        },{
> +            .name = "host-serial",
> +            .type = QEMU_OPT_STRING,
> +            .help = "host's system-id seen by guest",
> +        },{
> +            .name = "host-model",
> +            .type = QEMU_OPT_STRING,
> +            .help = "host's model-id seen by guest",
>          },
>          { /* End of list */ }
>      }

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]