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

["Daniel P. Berrangé" <berrange@redhat.com>]

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