Re: [PATCH v4] isa-applesmc: provide OSK forwarding on Apple hosts

[Daniel P . Berrangé]

On Mon, Oct 25, 2021 at 04:42:22PM +0200, Alexander Graf wrote:
> 
> On 25.10.21 16:22, Daniel P. Berrangé wrote:
> > On Mon, Oct 25, 2021 at 12:13:32PM +0200, Alexander Graf wrote:
> > > On 22.10.21 18:14, Vladislav Yaroshchuk wrote:
> > > > On Apple hosts we can read AppleSMC OSK key directly from host's
> > > > SMC and forward this value to QEMU Guest.
> > > > 
> > > > Usage:
> > > > `-device isa-applesmc,hostosk=on`
> > > > 
> > > > Apple licence allows use and run up to two additional copies
> > > > or instances of macOS operating within virtual operating system
> > > > environments on each Apple-branded computer that is already running
> > > > the Apple Software, for purposes of:
> > > > - software development
> > > > - testing during software development
> > > > - using macOS Server
> > > > - personal, non-commercial use
> > > > 
> > > > Guest macOS requires AppleSMC with correct OSK. The most legal
> > > > way to pass it to the Guest is to forward the key from host SMC
> > > > without any value exposion.
> > > > 
> > > > Based on 
> > > > https://web.archive.org/web/20200103161737/osxbook.com/book/bonus/chapter7/tpmdrmmyth/
> > > > 
> > > > Signed-off-by: Vladislav Yaroshchuk <yaroshchuk2000@gmail.com>
> > 
> > > > @@ -331,6 +464,25 @@ static void applesmc_isa_realize(DeviceState *dev, 
> > > > Error **errp)
> > > >        isa_register_ioport(&s->parent_obj, &s->io_err,
> > > >                            s->iobase + APPLESMC_ERR_PORT);
> > > > +    if (s->hostosk_flag) {
> > > > +        /*
> > > > +         * Property 'hostosk' has higher priority than 'osk'
> > > > +         * and shadows it.
> > > > +         * Free user-provided 'osk' property value
> > > > +         */
> > > > +        if (s->osk) {
> > > > +            warn_report("isa-applesmc.osk is shadowed "
> > > > +                        "by isa-applesmc.hostosk");
> > > > +            g_free(s->osk);
> > > > +        }
> > > > +
> > > > +        if (!applesmc_read_host_osk(&s->osk, &err)) {
> > > > +            /* On host OSK retrieval error report a warning */
> > > > +            error_report_err(err);
> > > > +            s->osk = default_osk;
> > > > +        }
> > > > +    }
> > > 
> > > This part is yucky. A few things:
> > > 
> > > 1) QEMU in general does not fail user requested operations silently. If 
> > > the
> > > user explicitly asked to read the host OSK and we couldn't, it must
> > > propagate that error.
> > > 2) In tandem to the above, I think the only consistent CX is to make both
> > > options mutually exclusive. The easiest way to achieve that IMHO would be 
> > > to
> > > overload the "osk" property. If it is "host", then use the host one.
> > > 3) Should we make "osk"="host" the default on macOS as well then? Of 
> > > course,
> > > that one should *not* fail hard when it can't read the key, because it's 
> > > an
> > > implicit request rather than an explicit one.
> > The problem with using a magic string value for the existing "osk"
> > parameter is that this is not introspectable by management apps.
> 
> 
> What introspectability would you like to have?

Essentially to answer the question

  "Does this QEMU support OSK passthrough from the host"

Mgmt apps like libvirt introspect using various query-XXX QMP commands.
For devices, the typical approach is to ask for the list of properties
the device supports. If we're just accepting a new magic value on an
existing property there is no way to query for existance of that feature.
If we add a "host-osk=bool" parameter introspectability is trivially
satisfied.

> > IMHO, using an explicit separate parameter is the right approach.
> > 
> > We just need to make sure we report an error properly via the
> > 'Error **errp' parameter to this method instead of warn_report
> > or error_report_err, when the user gives a non-sensible combination,
> > or if reading the host value fails.
> 
> 
> How about we change the default behavior altogether? Either you pass osk as
> argument, then that takes precedence. Or you don't, then we'll try to grab
> it from the host. If that fails, we error out (instead of the warn today).
> 
> That's a slight behavioral change, but I don't think anyone really wants to
> run the applesmc device without OSK in the first place. And at least we
> wouldn't break users that have working osk= configs.



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