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

[Laurent Vivier]

Le 22/10/2021 à 13:10, Vladislav Yaroshchuk a écrit :
> 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>
> ---
>   hw/misc/applesmc.c | 147 ++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 146 insertions(+), 1 deletion(-)
>
> diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
> index 1b9acaf1d3..bcc021f7b7 100644
> --- a/hw/misc/applesmc.c
> +++ b/hw/misc/applesmc.c
> @@ -38,6 +38,10 @@
>   #include "qemu/timer.h"
>   #include "qom/object.h"
>   
> +#if defined(__APPLE__)
> +#include <IOKit/IOKitLib.h>
> +#endif
> +
>   /* #define DEBUG_SMC */
>   
>   #define APPLESMC_DEFAULT_IOBASE        0x300
> @@ -108,6 +112,7 @@ struct AppleSMCState {
>       uint8_t data_len;
>       uint8_t data_pos;
>       uint8_t data[255];
> +    char *hostosk_flag;

Use a boolean (see below)

>       char *osk;
>       QLIST_HEAD(, AppleSMCData) data_def;
>   };
> @@ -312,9 +317,136 @@ static const MemoryRegionOps applesmc_err_io_ops = {
>       },
>   };
>   
> +#if defined(__APPLE__)
> +/*
> + * Based on
> + * 
> https://web.archive.org/web/20200103161737/osxbook.com/book/bonus/chapter7/tpmdrmmyth/
> + */
> +enum {
> +    SMC_CLIENT_OPEN      = 0,
> +    SMC_CLIENT_CLOSE     = 1,
> +    SMC_HANDLE_EVENT     = 2,
> +    SMC_READ_KEY         = 5
> +};
> +
> +struct AppleSMCParam {
> +    uint32_t    key;
> +    uint8_t     pad0[22];
> +    IOByteCount data_size;
> +    uint8_t     pad1[10];
> +    uint8_t     command;
> +    uint32_t    pad2;
> +    uint8_t     bytes[32];
> +};
> +
> +static int applesmc_read_host_osk(char **host_osk)

I think you should return the error message using an "Error **errp".

> +{
> +    assert(host_osk != NULL);
> +
> +    io_service_t            hostsmc_service = IO_OBJECT_NULL;
> +    io_connect_t            hostsmc_connect = IO_OBJECT_NULL;
> +    size_t                  out_size = sizeof(struct AppleSMCParam);
> +    IOReturn                status = kIOReturnError;
> +    struct AppleSMCParam    in = {0};
> +    struct AppleSMCParam    out = {0};
> +
> +    /* OSK key size + '\0' */
> +    *host_osk = g_malloc0(65);
> +    (*host_osk)[64] = '\0';
> +
> +    hostsmc_service = IOServiceGetMatchingService(kIOMasterPortDefault,
> +                                          IOServiceMatching("AppleSMC"));
> +    if (hostsmc_service == IO_OBJECT_NULL) {
> +        warn_report("host AppleSMC device is unreachable");
> +        goto error_osk_buffer_free;
> +    }
> +
> +    status = IOServiceOpen(hostsmc_service,
> +                           mach_task_self(),
> +                           1,
> +                           &hostsmc_connect);
> +    if (status != kIOReturnSuccess || hostsmc_connect == IO_OBJECT_NULL) {
> +        warn_report("host AppleSMC device is unreachable");
> +        goto error_osk_buffer_free;
> +    }
> +
> +    status = IOConnectCallMethod(
> +        hostsmc_connect,
> +        SMC_CLIENT_OPEN,
> +        NULL, 0, NULL, 0, NULL, NULL, NULL, NULL
> +    );
> +    if (status != kIOReturnSuccess) {
> +        warn_report("host AppleSMC device is unreachable");
> +        goto error_ioservice_close;
> +    }
> +
> +    in.key = ('OSK0');
> +    in.data_size = sizeof(out.bytes);
> +    in.command = SMC_READ_KEY;
> +    status = IOConnectCallStructMethod(
> +        hostsmc_connect,
> +        SMC_HANDLE_EVENT,
> +        &in,
> +        sizeof(struct AppleSMCParam),
> +        &out,
> +        &out_size
> +    );
> +
> +    if (status != kIOReturnSuccess) {
> +        warn_report("unable to read OSK0 from host AppleSMC device");
> +        goto error_ioconnect_close;
> +    }
> +    strncpy(*host_osk, (const char *) out.bytes, 32);
> +
> +    in.key = ('OSK1');
> +    in.data_size = sizeof(out.bytes);
> +    in.command = SMC_READ_KEY;
> +    status = IOConnectCallStructMethod(
> +        hostsmc_connect,
> +        SMC_HANDLE_EVENT,
> +        &in,
> +        sizeof(struct AppleSMCParam),
> +        &out,
> +        &out_size
> +    );
> +    if (status != kIOReturnSuccess) {
> +        warn_report("unable to read OSK1 from host AppleSMC device");
> +        goto error_ioconnect_close;
> +    }
> +    strncpy((*host_osk) + 32, (const char *) out.bytes, 32);
> +
> +    IOConnectCallMethod(
> +        hostsmc_connect,
> +        SMC_CLIENT_CLOSE,
> +        NULL, 0, NULL, 0, NULL, NULL, NULL, NULL);
> +    IOServiceClose(hostsmc_connect);
> +    return 0;
> +
> +error_ioconnect_close:
> +    IOConnectCallMethod(
> +        hostsmc_connect,
> +        SMC_CLIENT_CLOSE,
> +        NULL, 0, NULL, 0, NULL, NULL, NULL, NULL);
> +error_ioservice_close:
> +    IOServiceClose(hostsmc_connect);
> +
> +error_osk_buffer_free:
> +    g_free(*host_osk);
> +    return -1;
> +}
> +#else
> +static int applesmc_read_host_osk(char **output_key)
> +{
> +    warn_report("isa-applesmc.hostosk ignored: "
> +                "unsupported on non-Apple hosts");

I think a failure would be better than a warning.
Moreover will it work if the user doenst provide the OSK?

> +    return -1;
> +}
> +#endif
> +
>   static void applesmc_isa_realize(DeviceState *dev, Error **errp)
>   {
> -    AppleSMCState *s = APPLE_SMC(dev);
> +    AppleSMCState   *s = APPLE_SMC(dev);
> +    char            *host_osk;
>   
>       memory_region_init_io(&s->io_data, OBJECT(s), &applesmc_data_io_ops, s,
>                             "applesmc-data", 1);
> @@ -331,6 +463,18 @@ static void applesmc_isa_realize(DeviceState *dev, Error 
> **errp)
>       isa_register_ioport(&s->parent_obj, &s->io_err,
>                           s->iobase + APPLESMC_ERR_PORT);
>   
> +    /* Key retrieved from host SMC overwrites provided OSK string */
> +    if (s->hostosk_flag
> +        && !strcmp("on", s->hostosk_flag)

Use a bool property for hostosk (see below), that will allow "on", "yes", "true", ... and here 
you'll only have to test for the boolean value.

> +        && !applesmc_read_host_osk(&host_osk)) {
> +        if (s->osk) {
> +            warn_report("provided isa-applesmc.osk "
> +                        "is overwritten with host OSK");
> +            g_free(s->osk);
> +        }
> +        s->osk = host_osk;
> +    }
> +
>       if (!s->osk || (strlen(s->osk) != 64)) {
>           warn_report("Using AppleSMC with invalid key");
>           s->osk = default_osk;
> @@ -344,6 +488,7 @@ static Property applesmc_isa_properties[] = {
>       DEFINE_PROP_UINT32(APPLESMC_PROP_IO_BASE, AppleSMCState, iobase,
>                          APPLESMC_DEFAULT_IOBASE),
>       DEFINE_PROP_STRING("osk", AppleSMCState, osk),
> +    DEFINE_PROP_STRING("hostosk", AppleSMCState, hostosk_flag),

An DEFINE_PROP_BOOL() would be more appropriate for this.

>       DEFINE_PROP_END_OF_LIST(),
>   };
>   

Thanks,
Laurent