| Message ID | 20190218101218.4177-1-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] ppc: add host-serial and host-model machine attributes | expand |
On Mon, Feb 18, 2019 at 03:42:18PM +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> > --- > hw/ppc/spapr.c | 79 ++++++++++++++++++++++++++++++++++++++---- > include/hw/ppc/spapr.h | 2 ++ > 2 files changed, 75 insertions(+), 6 deletions(-) > > Update v3: move host-serial,host-model options to ppc sPAPR machine > -> https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg03182.html Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel
On Mon, 18 Feb 2019 15:42:18 +0530 P J P <ppandit@redhat.com> 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> > --- > hw/ppc/spapr.c | 79 ++++++++++++++++++++++++++++++++++++++---- > include/hw/ppc/spapr.h | 2 ++ > 2 files changed, 75 insertions(+), 6 deletions(-) > > Update v3: move host-serial,host-model options to ppc sPAPR machine > -> https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg03182.html > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 0942f35bf8..666e500376 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1249,13 +1249,30 @@ 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)) { > - _FDT(fdt_setprop_string(fdt, 0, "host-model", buf)); > - g_free(buf); > + 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 (kvmppc_get_host_serial(&buf)) { > - _FDT(fdt_setprop_string(fdt, 0, "host-serial", buf)); > - g_free(buf); > + > + 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)); > + } > } > > buf = qemu_uuid_unparse_strdup(&qemu_uuid); > @@ -3138,6 +3155,36 @@ static void spapr_set_ic_mode(Object *obj, const char *value, Error **errp) > } > } > > +static char *spapr_get_host_model(Object *obj, Error **errp) > +{ > + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > + > + return g_strdup(spapr->host_model); > +} > + > +static void spapr_set_host_model(Object *obj, const char *value, Error **errp) > +{ > + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > + > + g_free(spapr->host_model); > + spapr->host_model = g_strdup(value); > +} > + > +static char *spapr_get_host_serial(Object *obj, Error **errp) > +{ > + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > + > + return g_strdup(spapr->host_serial); > +} > + > +static void spapr_set_host_serial(Object *obj, const char *value, Error **errp) > +{ > + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > + > + g_free(spapr->host_serial); > + spapr->host_serial = g_strdup(value); > +} > + > static void spapr_instance_init(Object *obj) > { > sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > @@ -3183,6 +3230,20 @@ static void spapr_instance_init(Object *obj) > object_property_set_description(obj, "ic-mode", > "Specifies the interrupt controller mode (xics, xive, dual)", > NULL); > + > + spapr->host_model = NULL; This isn't needed since object_initialize_with_type() already takes care of zeroing the instance for us. > + object_property_add_str(obj, "host-model", > + 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); > + > + spapr->host_serial = NULL; Same here. > + 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); > } > > static void spapr_machine_finalizefn(Object *obj) > @@ -4080,9 +4141,15 @@ 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" }, > + }; > So... we don't fix the information leak for older machines by default ? From previous discussions, I understand it is for the sake of compatibility, but leaving the burden of securing the host to downstream or to the user still looks unsecure to me FWIW. > 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; > } > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index a947a0a0dc..e004a570d8 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -177,6 +177,8 @@ struct sPAPRMachineState { > > /*< public >*/ > char *kvm_type; > + char *host_model; > + char *host_serial; > > const char *icp_type; > int32_t irq_map_nr;
On Mon, Feb 18, 2019 at 12:38:11PM +0100, Greg Kurz wrote: > On Mon, 18 Feb 2019 15:42:18 +0530 > P J P <ppandit@redhat.com> 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> > > --- > > hw/ppc/spapr.c | 79 ++++++++++++++++++++++++++++++++++++++---- > > include/hw/ppc/spapr.h | 2 ++ > > 2 files changed, 75 insertions(+), 6 deletions(-) > > > > Update v3: move host-serial,host-model options to ppc sPAPR machine > > -> https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg03182.html > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 0942f35bf8..666e500376 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -1249,13 +1249,30 @@ 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)) { > > - _FDT(fdt_setprop_string(fdt, 0, "host-model", buf)); > > - g_free(buf); > > + 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 (kvmppc_get_host_serial(&buf)) { > > - _FDT(fdt_setprop_string(fdt, 0, "host-serial", buf)); > > - g_free(buf); > > + > > + 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)); > > + } > > } > > > > buf = qemu_uuid_unparse_strdup(&qemu_uuid); > > @@ -3138,6 +3155,36 @@ static void spapr_set_ic_mode(Object *obj, const char *value, Error **errp) > > } > > } > > > > +static char *spapr_get_host_model(Object *obj, Error **errp) > > +{ > > + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > > + > > + return g_strdup(spapr->host_model); > > +} > > + > > +static void spapr_set_host_model(Object *obj, const char *value, Error **errp) > > +{ > > + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > > + > > + g_free(spapr->host_model); > > + spapr->host_model = g_strdup(value); > > +} > > + > > +static char *spapr_get_host_serial(Object *obj, Error **errp) > > +{ > > + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > > + > > + return g_strdup(spapr->host_serial); > > +} > > + > > +static void spapr_set_host_serial(Object *obj, const char *value, Error **errp) > > +{ > > + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > > + > > + g_free(spapr->host_serial); > > + spapr->host_serial = g_strdup(value); > > +} > > + > > static void spapr_instance_init(Object *obj) > > { > > sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > > @@ -3183,6 +3230,20 @@ static void spapr_instance_init(Object *obj) > > object_property_set_description(obj, "ic-mode", > > "Specifies the interrupt controller mode (xics, xive, dual)", > > NULL); > > + > > + spapr->host_model = NULL; > > This isn't needed since object_initialize_with_type() already takes care > of zeroing the instance for us. > > > + object_property_add_str(obj, "host-model", > > + 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); > > + > > + spapr->host_serial = NULL; > > Same here. > > > + 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); > > } > > > > static void spapr_machine_finalizefn(Object *obj) > > @@ -4080,9 +4141,15 @@ 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" }, > > + }; > > > > So... we don't fix the information leak for older machines by default ? From > previous discussions, I understand it is for the sake of compatibility, but > leaving the burden of securing the host to downstream or to the user still > looks unsecure to me FWIW. Maintaining guest ABI compatibility has to take priority, even over fixing security issues, because we must never intentionally break guest OS/applications by silently altering guest ABI. This is one of the two reasons why machine type versioning exists (the other reason being live migration data stream). This is nothing new - we've done it before for security flaws where a fix would involve changing guest ABI. This particular security flaw is pretty minor compared to other cases that we've left unfixed by default eg Meltdown / Spectre and is easily addressed by the user if needed. Regards, Daniel
On Mon, 18 Feb 2019 11:52:18 +0000 Daniel P. Berrangé <berrange@redhat.com> wrote: > On Mon, Feb 18, 2019 at 12:38:11PM +0100, Greg Kurz wrote: > > On Mon, 18 Feb 2019 15:42:18 +0530 > > P J P <ppandit@redhat.com> 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> > > > --- > > > hw/ppc/spapr.c | 79 ++++++++++++++++++++++++++++++++++++++---- > > > include/hw/ppc/spapr.h | 2 ++ > > > 2 files changed, 75 insertions(+), 6 deletions(-) > > > > > > Update v3: move host-serial,host-model options to ppc sPAPR machine > > > -> https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg03182.html > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > index 0942f35bf8..666e500376 100644 > > > --- a/hw/ppc/spapr.c > > > +++ b/hw/ppc/spapr.c > > > @@ -1249,13 +1249,30 @@ 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)) { > > > - _FDT(fdt_setprop_string(fdt, 0, "host-model", buf)); > > > - g_free(buf); > > > + 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 (kvmppc_get_host_serial(&buf)) { > > > - _FDT(fdt_setprop_string(fdt, 0, "host-serial", buf)); > > > - g_free(buf); > > > + > > > + 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)); > > > + } > > > } > > > > > > buf = qemu_uuid_unparse_strdup(&qemu_uuid); > > > @@ -3138,6 +3155,36 @@ static void spapr_set_ic_mode(Object *obj, const char *value, Error **errp) > > > } > > > } > > > > > > +static char *spapr_get_host_model(Object *obj, Error **errp) > > > +{ > > > + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > > > + > > > + return g_strdup(spapr->host_model); > > > +} > > > + > > > +static void spapr_set_host_model(Object *obj, const char *value, Error **errp) > > > +{ > > > + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > > > + > > > + g_free(spapr->host_model); > > > + spapr->host_model = g_strdup(value); > > > +} > > > + > > > +static char *spapr_get_host_serial(Object *obj, Error **errp) > > > +{ > > > + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > > > + > > > + return g_strdup(spapr->host_serial); > > > +} > > > + > > > +static void spapr_set_host_serial(Object *obj, const char *value, Error **errp) > > > +{ > > > + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > > > + > > > + g_free(spapr->host_serial); > > > + spapr->host_serial = g_strdup(value); > > > +} > > > + > > > static void spapr_instance_init(Object *obj) > > > { > > > sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > > > @@ -3183,6 +3230,20 @@ static void spapr_instance_init(Object *obj) > > > object_property_set_description(obj, "ic-mode", > > > "Specifies the interrupt controller mode (xics, xive, dual)", > > > NULL); > > > + > > > + spapr->host_model = NULL; > > > > This isn't needed since object_initialize_with_type() already takes care > > of zeroing the instance for us. > > > > > + object_property_add_str(obj, "host-model", > > > + 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); > > > + > > > + spapr->host_serial = NULL; > > > > Same here. > > > > > + 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); > > > } > > > > > > static void spapr_machine_finalizefn(Object *obj) > > > @@ -4080,9 +4141,15 @@ 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" }, > > > + }; > > > > > > > So... we don't fix the information leak for older machines by default ? From > > previous discussions, I understand it is for the sake of compatibility, but > > leaving the burden of securing the host to downstream or to the user still > > looks unsecure to me FWIW. > > Maintaining guest ABI compatibility has to take priority, even over > fixing security issues, because we must never intentionally break > guest OS/applications by silently altering guest ABI. This is one of > the two reasons why machine type versioning exists (the other reason > being live migration data stream). > > This is nothing new - we've done it before for security flaws where > a fix would involve changing guest ABI. This particular security flaw > is pretty minor compared to other cases that we've left unfixed by > default eg Meltdown / Spectre and is easily addressed by the user if > needed. > > Regards, > Daniel Alright then if there's prior consensus on compatibility versus security. So, with or without the unneeded zeroing of the spapr->host_* fields: Reviewed-by: Greg Kurz <groug@kaod.org>
Hello Greg, Dan, +-- On Mon, 18 Feb 2019, Greg Kurz wrote --+ | >>> + spapr->host_model = NULL; | >> | >> This isn't needed since object_initialize_with_type() already takes care | >> of zeroing the instance for us. | >> | >>> + spapr->host_serial = NULL; | >> | >> Same here. | | Alright then if there's prior consensus on compatibility versus security. | | So, with or without the unneeded zeroing of the spapr->host_* fields: | | Reviewed-by: Greg Kurz <groug@kaod.org> I have sent a revised patch v4 removing above NULL initialisations. Thank you for the review. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
On Mon, Feb 18, 2019 at 11:52:18AM +0000, Daniel P. Berrangé wrote: > On Mon, Feb 18, 2019 at 12:38:11PM +0100, Greg Kurz wrote: > > On Mon, 18 Feb 2019 15:42:18 +0530 > > P J P <ppandit@redhat.com> 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> > > > --- > > > hw/ppc/spapr.c | 79 ++++++++++++++++++++++++++++++++++++++---- > > > include/hw/ppc/spapr.h | 2 ++ > > > 2 files changed, 75 insertions(+), 6 deletions(-) > > > > > > Update v3: move host-serial,host-model options to ppc sPAPR machine > > > -> https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg03182.html > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > index 0942f35bf8..666e500376 100644 > > > --- a/hw/ppc/spapr.c > > > +++ b/hw/ppc/spapr.c > > > @@ -1249,13 +1249,30 @@ 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)) { > > > - _FDT(fdt_setprop_string(fdt, 0, "host-model", buf)); > > > - g_free(buf); > > > + 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 (kvmppc_get_host_serial(&buf)) { > > > - _FDT(fdt_setprop_string(fdt, 0, "host-serial", buf)); > > > - g_free(buf); > > > + > > > + 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)); > > > + } > > > } > > > > > > buf = qemu_uuid_unparse_strdup(&qemu_uuid); > > > @@ -3138,6 +3155,36 @@ static void spapr_set_ic_mode(Object *obj, const char *value, Error **errp) > > > } > > > } > > > > > > +static char *spapr_get_host_model(Object *obj, Error **errp) > > > +{ > > > + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > > > + > > > + return g_strdup(spapr->host_model); > > > +} > > > + > > > +static void spapr_set_host_model(Object *obj, const char *value, Error **errp) > > > +{ > > > + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > > > + > > > + g_free(spapr->host_model); > > > + spapr->host_model = g_strdup(value); > > > +} > > > + > > > +static char *spapr_get_host_serial(Object *obj, Error **errp) > > > +{ > > > + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > > > + > > > + return g_strdup(spapr->host_serial); > > > +} > > > + > > > +static void spapr_set_host_serial(Object *obj, const char *value, Error **errp) > > > +{ > > > + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > > > + > > > + g_free(spapr->host_serial); > > > + spapr->host_serial = g_strdup(value); > > > +} > > > + > > > static void spapr_instance_init(Object *obj) > > > { > > > sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > > > @@ -3183,6 +3230,20 @@ static void spapr_instance_init(Object *obj) > > > object_property_set_description(obj, "ic-mode", > > > "Specifies the interrupt controller mode (xics, xive, dual)", > > > NULL); > > > + > > > + spapr->host_model = NULL; > > > > This isn't needed since object_initialize_with_type() already takes care > > of zeroing the instance for us. > > > > > + object_property_add_str(obj, "host-model", > > > + 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); > > > + > > > + spapr->host_serial = NULL; > > > > Same here. > > > > > + 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); > > > } > > > > > > static void spapr_machine_finalizefn(Object *obj) > > > @@ -4080,9 +4141,15 @@ 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" }, > > > + }; > > > > > > > So... we don't fix the information leak for older machines by default ? From > > previous discussions, I understand it is for the sake of compatibility, but > > leaving the burden of securing the host to downstream or to the user still > > looks unsecure to me FWIW. > > Maintaining guest ABI compatibility has to take priority, even over > fixing security issues, because we must never intentionally break > guest OS/applications by silently altering guest ABI. This is one of > the two reasons why machine type versioning exists (the other reason > being live migration data stream). > > This is nothing new - we've done it before for security flaws where > a fix would involve changing guest ABI. This particular security flaw > is pretty minor compared to other cases that we've left unfixed by > default eg Meltdown / Spectre and is easily addressed by the user if > needed. So, Markus was saying at the last KVM Forum - and I agree with him - that using "must maintain compatibility" as if its holy writ isn't actually very sensible. It's always a tradeoff, and we need to engage with that tradeoff on a case by case basis. In this case the security hole it fixes is pretty small - but the chances of realistically breaking the guests is also very small.
On Tue, Feb 19, 2019 at 12:21:04PM +1100, David Gibson wrote: > On Mon, Feb 18, 2019 at 11:52:18AM +0000, Daniel P. Berrangé wrote: > > On Mon, Feb 18, 2019 at 12:38:11PM +0100, Greg Kurz wrote: > > > On Mon, 18 Feb 2019 15:42:18 +0530 > > > P J P <ppandit@redhat.com> 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> > > > > --- > > > > hw/ppc/spapr.c | 79 ++++++++++++++++++++++++++++++++++++++---- > > > > include/hw/ppc/spapr.h | 2 ++ > > > > 2 files changed, 75 insertions(+), 6 deletions(-) > > > > > > > > Update v3: move host-serial,host-model options to ppc sPAPR machine > > > > -> https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg03182.html > > > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > > index 0942f35bf8..666e500376 100644 > > > > --- a/hw/ppc/spapr.c > > > > +++ b/hw/ppc/spapr.c > > > > @@ -1249,13 +1249,30 @@ 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)) { > > > > - _FDT(fdt_setprop_string(fdt, 0, "host-model", buf)); > > > > - g_free(buf); > > > > + 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 (kvmppc_get_host_serial(&buf)) { > > > > - _FDT(fdt_setprop_string(fdt, 0, "host-serial", buf)); > > > > - g_free(buf); > > > > + > > > > + 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)); > > > > + } > > > > } > > > > > > > > buf = qemu_uuid_unparse_strdup(&qemu_uuid); > > > > @@ -3138,6 +3155,36 @@ static void spapr_set_ic_mode(Object *obj, const char *value, Error **errp) > > > > } > > > > } > > > > > > > > +static char *spapr_get_host_model(Object *obj, Error **errp) > > > > +{ > > > > + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > > > > + > > > > + return g_strdup(spapr->host_model); > > > > +} > > > > + > > > > +static void spapr_set_host_model(Object *obj, const char *value, Error **errp) > > > > +{ > > > > + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > > > > + > > > > + g_free(spapr->host_model); > > > > + spapr->host_model = g_strdup(value); > > > > +} > > > > + > > > > +static char *spapr_get_host_serial(Object *obj, Error **errp) > > > > +{ > > > > + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > > > > + > > > > + return g_strdup(spapr->host_serial); > > > > +} > > > > + > > > > +static void spapr_set_host_serial(Object *obj, const char *value, Error **errp) > > > > +{ > > > > + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > > > > + > > > > + g_free(spapr->host_serial); > > > > + spapr->host_serial = g_strdup(value); > > > > +} > > > > + > > > > static void spapr_instance_init(Object *obj) > > > > { > > > > sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > > > > @@ -3183,6 +3230,20 @@ static void spapr_instance_init(Object *obj) > > > > object_property_set_description(obj, "ic-mode", > > > > "Specifies the interrupt controller mode (xics, xive, dual)", > > > > NULL); > > > > + > > > > + spapr->host_model = NULL; > > > > > > This isn't needed since object_initialize_with_type() already takes care > > > of zeroing the instance for us. > > > > > > > + object_property_add_str(obj, "host-model", > > > > + 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); > > > > + > > > > + spapr->host_serial = NULL; > > > > > > Same here. > > > > > > > + 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); > > > > } > > > > > > > > static void spapr_machine_finalizefn(Object *obj) > > > > @@ -4080,9 +4141,15 @@ 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" }, > > > > + }; > > > > > > > > > > So... we don't fix the information leak for older machines by default ? From > > > previous discussions, I understand it is for the sake of compatibility, but > > > leaving the burden of securing the host to downstream or to the user still > > > looks unsecure to me FWIW. > > > > Maintaining guest ABI compatibility has to take priority, even over > > fixing security issues, because we must never intentionally break > > guest OS/applications by silently altering guest ABI. This is one of > > the two reasons why machine type versioning exists (the other reason > > being live migration data stream). > > > > This is nothing new - we've done it before for security flaws where > > a fix would involve changing guest ABI. This particular security flaw > > is pretty minor compared to other cases that we've left unfixed by > > default eg Meltdown / Spectre and is easily addressed by the user if > > needed. > > So, Markus was saying at the last KVM Forum - and I agree with him - > that using "must maintain compatibility" as if its holy writ isn't > actually very sensible. It's always a tradeoff, and we need to engage > with that tradeoff on a case by case basis. > > In this case the security hole it fixes is pretty small - but the > chances of realistically breaking the guests is also very small. There's many different areas of QEMU were maintaining compatibility is relevant and they are varying degrees of importance in their effect. HMP is one where we've been most flexible in accepting incompatible changes since we explicitly don't promise stability. We are stricter for CLI and QMP, requiring deprecation cycle for incompatibilities unless we can see the change in question can't affect users (because it was already long broken or impossible to use). Guest ABI and migration stream are areas where we strive to never (knowingly) make incompatibile changes. When we do make guest ABI changes, we always tie them into the machine type version. I don't see any compelling reason to diverge from our historic practice and knowingly break guest ABI in already released machine types, as the cost of doing the preserving ABI is negligible. Regards, Daniel
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 0942f35bf8..666e500376 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1249,13 +1249,30 @@ 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)) { - _FDT(fdt_setprop_string(fdt, 0, "host-model", buf)); - g_free(buf); + 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 (kvmppc_get_host_serial(&buf)) { - _FDT(fdt_setprop_string(fdt, 0, "host-serial", buf)); - g_free(buf); + + 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)); + } } buf = qemu_uuid_unparse_strdup(&qemu_uuid); @@ -3138,6 +3155,36 @@ static void spapr_set_ic_mode(Object *obj, const char *value, Error **errp) } } +static char *spapr_get_host_model(Object *obj, Error **errp) +{ + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); + + return g_strdup(spapr->host_model); +} + +static void spapr_set_host_model(Object *obj, const char *value, Error **errp) +{ + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); + + g_free(spapr->host_model); + spapr->host_model = g_strdup(value); +} + +static char *spapr_get_host_serial(Object *obj, Error **errp) +{ + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); + + return g_strdup(spapr->host_serial); +} + +static void spapr_set_host_serial(Object *obj, const char *value, Error **errp) +{ + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); + + g_free(spapr->host_serial); + spapr->host_serial = g_strdup(value); +} + static void spapr_instance_init(Object *obj) { sPAPRMachineState *spapr = SPAPR_MACHINE(obj); @@ -3183,6 +3230,20 @@ static void spapr_instance_init(Object *obj) object_property_set_description(obj, "ic-mode", "Specifies the interrupt controller mode (xics, xive, dual)", NULL); + + spapr->host_model = NULL; + object_property_add_str(obj, "host-model", + 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); + + spapr->host_serial = NULL; + 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); } static void spapr_machine_finalizefn(Object *obj) @@ -4080,9 +4141,15 @@ 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; } diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index a947a0a0dc..e004a570d8 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -177,6 +177,8 @@ struct sPAPRMachineState { /*< public >*/ char *kvm_type; + char *host_model; + char *host_serial; const char *icp_type; int32_t irq_map_nr;