пн, 25 окт. 2021 г. в 13:13, Alexander Graf <ag...@csgraf.de>:
> > 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 <yaroshchuk2...@gmail.com> > > > There was a similar proposal on the mailing list by Pedro Torres > recently. Please copy everyone who contributed to that email thread in > your patch submission in addition to everyone who commented on previous > versions of your own submission. > > I understood that I'd missed the already submitted patch by Pedro Torres. I should have queried 'applesmc' instead of 'isa-applesmc' to find his patch. I've checked the mailing discussion and found some useful things to fix, thank you. > Also, this is definitely not "trivial" material. Trivial means spelling > or super obvious fixes. This one is an actual feature :). I've removed > qemu-trivial@ from the copy list. > > My mistake, won't make the same next time :) > > > --- > > hw/misc/applesmc.c | 155 ++++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 154 insertions(+), 1 deletion(-) > > > > diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c > > index 1b9acaf1d3..6bdec0063b 100644 > > --- a/hw/misc/applesmc.c > > +++ b/hw/misc/applesmc.c > > @@ -37,6 +37,11 @@ > > #include "qemu/module.h" > > #include "qemu/timer.h" > > #include "qom/object.h" > > +#include "qapi/error.h" > > + > > +#if defined(__APPLE__) > > +#include <IOKit/IOKitLib.h> > > +#endif > > > > /* #define DEBUG_SMC */ > > > > @@ -108,6 +113,7 @@ struct AppleSMCState { > > uint8_t data_len; > > uint8_t data_pos; > > uint8_t data[255]; > > + bool hostosk_flag; > > char *osk; > > QLIST_HEAD(, AppleSMCData) data_def; > > }; > > @@ -312,9 +318,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 bool applesmc_read_host_osk(char **host_osk, Error **errp) > > > Please just provide a buffer to write the OSK into (char *host_osk) to > the function. > > Thank you, I will fix this. > > > +{ > > + 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}; > > > Can in and out be the same variable? > > Checked, we can use one variable for in and out. > > + > > + /* 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) { > > + error_setg(errp, "Unable to get host-AppleSMC service"); > > + goto error_osk_buffer_free; > > + } > > + > > + status = IOServiceOpen(hostsmc_service, > > + mach_task_self(), > > + 1, > > + &hostsmc_connect); > > + if (status != kIOReturnSuccess || hostsmc_connect == > IO_OBJECT_NULL) { > > + error_setg(errp, "Unable to open host-AppleSMC service"); > > + goto error_osk_buffer_free; > > + } > > + > > + status = IOConnectCallMethod( > > + hostsmc_connect, > > + SMC_CLIENT_OPEN, > > + NULL, 0, NULL, 0, NULL, NULL, NULL, NULL > > + ); > > + if (status != kIOReturnSuccess) { > > + error_setg(errp, "Unable to connect to host-AppleSMC"); > > + goto error_ioservice_close; > > + } > > + > > + in.key = ('OSK0'); > > + in.data_size = sizeof(out.bytes); > > + in.command = SMC_READ_KEY; > > > Since you know all these beforehand, can't you just create two struct > AppleSMCParam - one for osk0 and one for osk1? Then just shoot them off > here: > > struct AppleSMCParam cmd[2] = { > { > .key = ('OSK0'), > .data_size = sizeof(cmd[0].bytes), > .command = SMC_READ_KEY, > }, { > .key = ('OSK1'), > .data_size = sizeof(cmd[0].bytes), > .command = SMC_READ_KEY, > }, > }; > > > for (i = 0; i < ARRAY_SIZE(in); i++) { > IOConnectCallStructMethod(hostsmc_connect, SMC_HANDLE_EVENT, > cmd[i], sizeof(cmd[i]), cmd[i], &out_size); > if (out_size != sizeof(cmd[i])) { > goto err; > } > } > > A good idea, I'll rewrite the code using this approach, thank you! > > + status = IOConnectCallStructMethod( > > + hostsmc_connect, > > + SMC_HANDLE_EVENT, > > + &in, > > + sizeof(struct AppleSMCParam), > > + &out, > > + &out_size > > + ); > > + > > + if (status != kIOReturnSuccess) { > > + error_setg(errp, "Unable to read OSK0 from host-AppleSMC"); > > + goto error_ioconnect_close; > > + } > > + strncpy(*host_osk, (const char *) out.bytes, 32); > > > The OSK is not a string (well, it is, but technically it isn't). Just > treat it as opaque 32 bytes of memory. Memcpy is what you want here. > > Yes, agree with you, fixing it now. > > + > > + 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) { > > + error_setg(errp, "Unable to read OSK1 from host-AppleSMC"); > > + 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 true; > > + > > +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 false; > > +} > > +#else > > +static bool applesmc_read_host_osk(char **output_key, Error **errp) > > +{ > > + error_setg(errp, "isa-applesmc.hostosk ignored: " > > + "unsupported on non-Apple hosts"); > > + return false; > > +} > > +#endif > > + > > static void applesmc_isa_realize(DeviceState *dev, Error **errp) > > { > > - AppleSMCState *s = APPLE_SMC(dev); > > + AppleSMCState *s = APPLE_SMC(dev); > > Why the whitespace change here? > > It was used to align `*s` and `*err`. Seems disaccorded with QEMU code style, I'll remove the space in the next patch. > > + Error *err = NULL; > > > > memory_region_init_io(&s->io_data, OBJECT(s), > &applesmc_data_io_ops, s, > > "applesmc-data", 1); > > @@ -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. > Sounds reasonable, let's use `osk=host`, drop `hostosk=on` and enable this feature on Apple-macOS hosts by default. > Alex > > > > + > > if (!s->osk || (strlen(s->osk) != 64)) { > > warn_report("Using AppleSMC with invalid key"); > > s->osk = default_osk; > > @@ -344,6 +496,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_BOOL("hostosk", AppleSMCState, hostosk_flag, false), > > DEFINE_PROP_END_OF_LIST(), > > }; > > -- Best Regards, Vladislav Yaroshchuk
