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

[Daniel P . Berrangé]

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 <address@hidden> wrote:
> > > 
> > > > From: Prasad J Pandit <address@hidden>
> > > > 
> > > > 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é <address@hidden>
> > > > Fix-suggested-by: Daniel P. Berrangé <address@hidden>
> > > > Signed-off-by: Prasad J Pandit <address@hidden>
> > > > ---
> > > >  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
-- 
|: 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 :|