Hi, Laurent! Thank you for your review! пт, 22 окт. 2021 г. в 14:36, Laurent Vivier <laur...@vivier.eu>:
> 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 <yaroshchuk2...@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". Yep, sounds much better, will fix this in the next patch version. > +{ > > + 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? > > Not sure that failure is more suitable than warning. See below [0]. > + 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; > [0] The behavior of `osk` property handle: when the wrong OSK is provided (or not provided at all) isa-applesmc uses `default_osk` and continues to work fine. Only a warning is printed. Seems it's better to meet this "rule": when we can't read OSK from host-SMC just warn the user and continue with `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. > > Will fix the property in the next patch version, thank you! > DEFINE_PROP_END_OF_LIST(), > > }; > > > > > > Thanks, > Laurent > > -- Best Regards, Vladislav Yaroshchuk
