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

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

[P J P]

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>
---
 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 */ }
     }
-- 
2.20.1

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

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190201185358.6972-1-ppandit@redhat.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=14
=== TEST SCRIPT END ===


Configure options:
--enable-werror --target-list=x86_64-softmmu,aarch64-softmmu --prefix=/tmp/qemu-test/install --python=/usr/bin/python3 --cross-prefix=x86_64-w64-mingw32- --enable-trace-backends=simple --enable-gnutls --enable-nettle --enable-curl --enable-vnc --enable-bzip2 --enable-guest-agent --with-sdlabi=2.0
ERROR: unknown option --with-sdlabi=2.0
Try '/tmp/qemu-test/src/configure --help' for more information
# QEMU configure log Sun Feb  3 16:10:42 UTC 2019
# Configured with: '/tmp/qemu-test/src/configure' '--enable-werror' '--target-list=x86_64-softmmu,aarch64-softmmu' '--prefix=/tmp/qemu-test/install' '--python=/usr/bin/python3' '--cross-prefix=x86_64-w64-mingw32-' '--enable-trace-backends=simple' '--enable-gnutls' '--enable-nettle' '--enable-curl' '--enable-vnc' '--enable-bzip2' '--enable-guest-agent' '--with-sdlabi=2.0'
---
funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 617 634 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
config-temp/qemu-conf.c:2:2: error: #error __linux__ not defined
 #error __linux__ not defined
  ^~~~~

---
funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 617 686 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
config-temp/qemu-conf.c:2:2: error: #error __i386__ not defined
 #error __i386__ not defined
  ^~~~~

---
funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 617 689 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
config-temp/qemu-conf.c:2:2: error: #error __ILP32__ not defined
 #error __ILP32__ not defined
  ^~~~~

---
lines: 92 128 920 0
x86_64-w64-mingw32-gcc -mthreads -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -o config-temp/qemu-conf.exe config-temp/qemu-conf.c -g -liberty
/usr/lib/gcc/x86_64-w64-mingw32/8.2.0/../../../../x86_64-w64-mingw32/bin/ld: cannot find -liberty
collect2: error: ld returned 1 exit status
Failed to run 'configure'
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 563, in <module>


The full log is available at
http://patchew.org/logs/20190201185358.6972-1-ppandit@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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

[David Gibson]

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

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

[P J P]

+-- On Mon, 4 Feb 2019, David Gibson wrote --+
| I'm wondering if we can just ditch them entirely, or at least make
| them default to not present without regard to machine version.

Ie. make the default behaviour host-serial/host-model=NULL/none, instead of 
'passthrough' now?

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

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

[David Gibson]

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

On Mon, Feb 04, 2019 at 11:40:46AM +0530, P J P wrote:
> +-- On Mon, 4 Feb 2019, David Gibson wrote --+
> | I'm wondering if we can just ditch them entirely, or at least make
> | them default to not present without regard to machine version.
> 
> Ie. make the default behaviour host-serial/host-model=NULL/none, instead of 
> 'passthrough' now?

Yes.

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

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

[P J P]

+-- On Mon, 4 Feb 2019, David Gibson wrote --+
| On Mon, Feb 04, 2019 at 11:40:46AM +0530, P J P wrote:
| > Ie. make the default behaviour host-serial/host-model=NULL/none, instead of 
| > 'passthrough' now?
| 
| Yes.
| 
Okay, I'll send a revised patch. Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

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

[Daniel P. Berrangé]

On Mon, Feb 04, 2019 at 12:09:04PM +1100, David Gibson wrote:
> 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.

We cannot assume that no one is using it.

In fact this issue came to light precisely because a person on IRC
was asking why x86 couldn't provide the same info as PPC, because
they found it useful on PPC.

So we will definitely break people if we remove this from existing
VMs.

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

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

[David Gibson]

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

On Mon, Feb 04, 2019 at 10:10:05AM +0000, Daniel P. Berrangé wrote:
> On Mon, Feb 04, 2019 at 12:09:04PM +1100, David Gibson wrote:
> > 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.
> 
> We cannot assume that no one is using it.
> 
> In fact this issue came to light precisely because a person on IRC
> was asking why x86 couldn't provide the same info as PPC, because
> they found it useful on PPC.

"Found it useful" is not the same as actually relying on.

> So we will definitely break people if we remove this from existing
> VMs.

I don't think that follows from the information you've presented so
far.

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

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

[Daniel P. Berrangé]

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>


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

buf hasn't been initialized

> +        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);
>      }

This structure for the conditionals is a bit unreadable IMHO. It would
be better as a nested if

     if (machine->host_model && !g_str_equal(machine->host_model, "none")) {
         if (g_str_equal(machine->host_model, "passthrough") {
      	     if (!kvmppc_get_host_model(&buf)) {
	         ... report error...
             }
	    _FDT(fdt_setprop_string(fdt, 0, "host-model", buf));
	    g_free(buf);
	 } else {
	    _FDT(fdt_setprop_string(fdt, 0, "host-model", machine->host_model));
	 }
     }



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

Same comment for this block.


There's missing logic to set host-model=passthrough for existing machine
types too.

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