* [PATCH v4] isa-applesmc: provide OSK forwarding on Apple hosts @ 2021-10-22 16:14 Vladislav Yaroshchuk 2021-10-25 10:13 ` Alexander Graf 0 siblings, 1 reply; 11+ messages in thread From: Vladislav Yaroshchuk @ 2021-10-22 16:14 UTC (permalink / raw) To: qemu-devel Cc: Vladislav Yaroshchuk, qemu-trivial, laurent, agraf, r.bolshakov, suse, f4bug 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 | 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) +{ + 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) { + 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; + 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); + + 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); + 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; + } + } + 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(), }; -- 2.23.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v4] isa-applesmc: provide OSK forwarding on Apple hosts 2021-10-22 16:14 [PATCH v4] isa-applesmc: provide OSK forwarding on Apple hosts Vladislav Yaroshchuk @ 2021-10-25 10:13 ` Alexander Graf 2021-10-25 14:14 ` Vladislav Yaroshchuk 2021-10-25 14:22 ` Daniel P. Berrangé 0 siblings, 2 replies; 11+ messages in thread From: Alexander Graf @ 2021-10-25 10:13 UTC (permalink / raw) To: Vladislav Yaroshchuk, qemu-devel Cc: Phil Dennis-Jordan, Pedro Tôrres, laurent, f4bug, r.bolshakov, Paolo Bonzini, Gabriel L. Somlo, suse 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> 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. 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. > --- > 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. > +{ > + 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? > + > + /* 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; } } > + 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. > + > + 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? > + 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. 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(), > }; > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] isa-applesmc: provide OSK forwarding on Apple hosts 2021-10-25 10:13 ` Alexander Graf @ 2021-10-25 14:14 ` Vladislav Yaroshchuk 2021-10-25 14:22 ` Daniel P. Berrangé 1 sibling, 0 replies; 11+ messages in thread From: Vladislav Yaroshchuk @ 2021-10-25 14:14 UTC (permalink / raw) To: Alexander Graf, Laurent Vivier, Roman Bolshakov, suse, Philippe Mathieu-Daudé, Pedro Tôrres, Paolo Bonzini, Phil Dennis-Jordan, Gabriel L. Somlo, BALATON Zoltan, qemu-devel [-- Attachment #1: Type: text/plain, Size: 11043 bytes --] пн, 25 окт. 2021 г. в 13:13, Alexander Graf <agraf@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 <yaroshchuk2000@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 [-- Attachment #2: Type: text/html, Size: 15917 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] isa-applesmc: provide OSK forwarding on Apple hosts 2021-10-25 10:13 ` Alexander Graf 2021-10-25 14:14 ` Vladislav Yaroshchuk @ 2021-10-25 14:22 ` Daniel P. Berrangé 2021-10-25 14:42 ` Alexander Graf 1 sibling, 1 reply; 11+ messages in thread From: Daniel P. Berrangé @ 2021-10-25 14:22 UTC (permalink / raw) To: Alexander Graf Cc: Phil Dennis-Jordan, Pedro Tôrres, Vladislav Yaroshchuk, suse, f4bug, qemu-devel, r.bolshakov, Gabriel L. Somlo, Paolo Bonzini, laurent 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. 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. 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 :| ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] isa-applesmc: provide OSK forwarding on Apple hosts 2021-10-25 14:22 ` Daniel P. Berrangé @ 2021-10-25 14:42 ` Alexander Graf 2021-10-25 14:47 ` Daniel P. Berrangé 0 siblings, 1 reply; 11+ messages in thread From: Alexander Graf @ 2021-10-25 14:42 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Phil Dennis-Jordan, Pedro Tôrres, Vladislav Yaroshchuk, suse, f4bug, qemu-devel, r.bolshakov, Gabriel L. Somlo, Paolo Bonzini, laurent 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? > 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. Alex ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] isa-applesmc: provide OSK forwarding on Apple hosts 2021-10-25 14:42 ` Alexander Graf @ 2021-10-25 14:47 ` Daniel P. Berrangé 2021-10-25 14:53 ` Alexander Graf 0 siblings, 1 reply; 11+ messages in thread From: Daniel P. Berrangé @ 2021-10-25 14:47 UTC (permalink / raw) To: Alexander Graf Cc: Phil Dennis-Jordan, Pedro Tôrres, Vladislav Yaroshchuk, suse, f4bug, qemu-devel, r.bolshakov, Gabriel L. Somlo, Paolo Bonzini, laurent 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 :| ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] isa-applesmc: provide OSK forwarding on Apple hosts 2021-10-25 14:47 ` Daniel P. Berrangé @ 2021-10-25 14:53 ` Alexander Graf 2021-10-25 15:10 ` Daniel P. Berrangé 0 siblings, 1 reply; 11+ messages in thread From: Alexander Graf @ 2021-10-25 14:53 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Phil Dennis-Jordan, Pedro Tôrres, Vladislav Yaroshchuk, suse, f4bug, qemu-devel, r.bolshakov, Gabriel L. Somlo, Paolo Bonzini, laurent On 25.10.21 16:47, Daniel P. Berrangé wrote: > 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. Ok, the only flow that remains sensible in that case to me sounds like the following: if (s->osk) { /* Use osk */ } else if (s->use_host_osk) { /* Use host OSK. Fail hard if we can't find it */ } else if (can_use_host_osk) { /* See if we can extract the key from the host. If not, fall back to old behavior */ } else { /* Old fallback behavior */ } Alex ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] isa-applesmc: provide OSK forwarding on Apple hosts 2021-10-25 14:53 ` Alexander Graf @ 2021-10-25 15:10 ` Daniel P. Berrangé 2021-10-25 19:45 ` Alexander Graf 0 siblings, 1 reply; 11+ messages in thread From: Daniel P. Berrangé @ 2021-10-25 15:10 UTC (permalink / raw) To: Alexander Graf Cc: Phil Dennis-Jordan, Pedro Tôrres, Vladislav Yaroshchuk, suse, f4bug, qemu-devel, r.bolshakov, Gabriel L. Somlo, Paolo Bonzini, laurent On Mon, Oct 25, 2021 at 04:53:57PM +0200, Alexander Graf wrote: > > On 25.10.21 16:47, Daniel P. Berrangé wrote: > > 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. > > > Ok, the only flow that remains sensible in that case to me sounds like the > following: Just need an extra check upfront: if (s->osk && s->use_hoist_osk) error_setg(errp, ...) else > if (s->osk) { > /* Use osk */ This should fail hard if the provided value is the wrong length - currently it falls back with a warning IIUC. > } else if (s->use_host_osk) { > /* Use host OSK. Fail hard if we can't find it */ > } else if (can_use_host_osk) { > /* See if we can extract the key from the host. If not, fall back to old > behavior */ > } else { > /* Old fallback behavior */ Was this old fallback behaviour actually useful ? IIUC it means it is using static char default_osk[64] = "This is a dummy key. Enter the real key " "using the -osk parameter"; which obviously isn't a valid key that will work with any gust OS that cares. I guess it at least let QEMU startup, but any the guest OS that checks the key will be unhappy. If if don't think default_osk is actually useful, then we could simplify further to if (s->osk && s->use_host_osk) { error_setg(errp, ...) } else if (s->osk) { /* Use osk. Fail hard if invalid (ie wrong length) */ } else if (s->use_host_osk) { /* Use host OSK. Fail hard if we can't find it */ } else { /* try to use host OSK, fail hard if we can't find it or non-OS-X build */ } 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 :| ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] isa-applesmc: provide OSK forwarding on Apple hosts 2021-10-25 15:10 ` Daniel P. Berrangé @ 2021-10-25 19:45 ` Alexander Graf 2021-10-26 8:42 ` Daniel P. Berrangé 0 siblings, 1 reply; 11+ messages in thread From: Alexander Graf @ 2021-10-25 19:45 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Phil Dennis-Jordan, Pedro Tôrres, Vladislav Yaroshchuk, suse, f4bug, qemu-devel, r.bolshakov, Gabriel L. Somlo, Paolo Bonzini, laurent On 25.10.21 17:10, Daniel P. Berrangé wrote: > On Mon, Oct 25, 2021 at 04:53:57PM +0200, Alexander Graf wrote: >> On 25.10.21 16:47, Daniel P. Berrangé wrote: >>> 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. >> >> Ok, the only flow that remains sensible in that case to me sounds like the >> following: > Just need an extra check upfront: > > if (s->osk && s->use_hoist_osk) > error_setg(errp, ...) > else > >> if (s->osk) { >> /* Use osk */ > This should fail hard if the provided value is the wrong length - currently > it falls back with a warning IIUC. > >> } else if (s->use_host_osk) { >> /* Use host OSK. Fail hard if we can't find it */ >> } else if (can_use_host_osk) { >> /* See if we can extract the key from the host. If not, fall back to old >> behavior */ >> } else { >> /* Old fallback behavior */ > Was this old fallback behaviour actually useful ? IIUC it means it is using > > > static char default_osk[64] = "This is a dummy key. Enter the real key " > "using the -osk parameter"; > > which obviously isn't a valid key that will work with any gust OS that > cares. I guess it at least let QEMU startup, but any the guest OS that > checks the key will be unhappy. > > If if don't think default_osk is actually useful, then we could simplify > further to > > if (s->osk && s->use_host_osk) { > error_setg(errp, ...) > } else if (s->osk) { > /* Use osk. Fail hard if invalid (ie wrong length) */ > } else if (s->use_host_osk) { > /* Use host OSK. Fail hard if we can't find it */ > } else { > /* try to use host OSK, fail hard if we can't find it or non-OS-X build */ > } In the example above, use_host_osk=on and use_host_osk=off yield the exact same behavior, so we don't need the switch, no? Alex ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] isa-applesmc: provide OSK forwarding on Apple hosts 2021-10-25 19:45 ` Alexander Graf @ 2021-10-26 8:42 ` Daniel P. Berrangé 2021-10-26 10:41 ` Alexander Graf 0 siblings, 1 reply; 11+ messages in thread From: Daniel P. Berrangé @ 2021-10-26 8:42 UTC (permalink / raw) To: Alexander Graf Cc: Phil Dennis-Jordan, Pedro Tôrres, Vladislav Yaroshchuk, suse, f4bug, qemu-devel, r.bolshakov, Gabriel L. Somlo, Paolo Bonzini, laurent On Mon, Oct 25, 2021 at 09:45:35PM +0200, Alexander Graf wrote: > > On 25.10.21 17:10, Daniel P. Berrangé wrote: > > On Mon, Oct 25, 2021 at 04:53:57PM +0200, Alexander Graf wrote: > > > On 25.10.21 16:47, Daniel P. Berrangé wrote: > > > > 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. > > > > > > Ok, the only flow that remains sensible in that case to me sounds like the > > > following: > > Just need an extra check upfront: > > > > if (s->osk && s->use_hoist_osk) > > error_setg(errp, ...) > > else > > > if (s->osk) { > > > /* Use osk */ > > This should fail hard if the provided value is the wrong length - currently > > it falls back with a warning IIUC. > > > > > } else if (s->use_host_osk) { > > > /* Use host OSK. Fail hard if we can't find it */ > > > } else if (can_use_host_osk) { > > > /* See if we can extract the key from the host. If not, fall back to old > > > behavior */ > > > } else { > > > /* Old fallback behavior */ > > Was this old fallback behaviour actually useful ? IIUC it means it is using > > > > > > static char default_osk[64] = "This is a dummy key. Enter the real key " > > "using the -osk parameter"; > > > > which obviously isn't a valid key that will work with any gust OS that > > cares. I guess it at least let QEMU startup, but any the guest OS that > > checks the key will be unhappy. > > > > If if don't think default_osk is actually useful, then we could simplify > > further to > > > > if (s->osk && s->use_host_osk) { > > error_setg(errp, ...) > > } else if (s->osk) { > > /* Use osk. Fail hard if invalid (ie wrong length) */ > > } else if (s->use_host_osk) { > > /* Use host OSK. Fail hard if we can't find it */ > > } else { > > /* try to use host OSK, fail hard if we can't find it or non-OS-X build */ > > } > > > In the example above, use_host_osk=on and use_host_osk=off yield the exact > same behavior, so we don't need the switch, no? Hmm, I forgot about machine type back compat. From a strict pov, if we change the default it should only be for the 6.2.0 machine type, with old machines keeping the current 'default_osk' behaviour. IOW, for 6.2.0 machine use_host_osk=on as default, and for < 6.2.0, use_host_osk=off as default . 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 :| ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] isa-applesmc: provide OSK forwarding on Apple hosts 2021-10-26 8:42 ` Daniel P. Berrangé @ 2021-10-26 10:41 ` Alexander Graf 0 siblings, 0 replies; 11+ messages in thread From: Alexander Graf @ 2021-10-26 10:41 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Phil Dennis-Jordan, Pedro Tôrres, Vladislav Yaroshchuk, suse, f4bug, qemu-devel, r.bolshakov, Gabriel L. Somlo, Paolo Bonzini, laurent On 26.10.21 10:42, Daniel P. Berrangé wrote: > On Mon, Oct 25, 2021 at 09:45:35PM +0200, Alexander Graf wrote: >> On 25.10.21 17:10, Daniel P. Berrangé wrote: >>> On Mon, Oct 25, 2021 at 04:53:57PM +0200, Alexander Graf wrote: >>>> On 25.10.21 16:47, Daniel P. Berrangé wrote: >>>>> 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. >>>> Ok, the only flow that remains sensible in that case to me sounds like the >>>> following: >>> Just need an extra check upfront: >>> >>> if (s->osk && s->use_hoist_osk) >>> error_setg(errp, ...) >>> else >>>> if (s->osk) { >>>> /* Use osk */ >>> This should fail hard if the provided value is the wrong length - currently >>> it falls back with a warning IIUC. >>> >>>> } else if (s->use_host_osk) { >>>> /* Use host OSK. Fail hard if we can't find it */ >>>> } else if (can_use_host_osk) { >>>> /* See if we can extract the key from the host. If not, fall back to old >>>> behavior */ >>>> } else { >>>> /* Old fallback behavior */ >>> Was this old fallback behaviour actually useful ? IIUC it means it is using >>> >>> >>> static char default_osk[64] = "This is a dummy key. Enter the real key " >>> "using the -osk parameter"; >>> >>> which obviously isn't a valid key that will work with any gust OS that >>> cares. I guess it at least let QEMU startup, but any the guest OS that >>> checks the key will be unhappy. >>> >>> If if don't think default_osk is actually useful, then we could simplify >>> further to >>> >>> if (s->osk && s->use_host_osk) { >>> error_setg(errp, ...) >>> } else if (s->osk) { >>> /* Use osk. Fail hard if invalid (ie wrong length) */ >>> } else if (s->use_host_osk) { >>> /* Use host OSK. Fail hard if we can't find it */ >>> } else { >>> /* try to use host OSK, fail hard if we can't find it or non-OS-X build */ >>> } >> >> In the example above, use_host_osk=on and use_host_osk=off yield the exact >> same behavior, so we don't need the switch, no? > Hmm, I forgot about machine type back compat. From a strict pov, if we > change the default it should only be for the 6.2.0 machine type, with > old machines keeping the current 'default_osk' behaviour. > > IOW, for 6.2.0 machine use_host_osk=on as default, and for < 6.2.0, > use_host_osk=off as default . Yes, and then the fallback case (no osk, no use_host_osk) would keep installing the dummy key. However, it means that if you create an applesmc VM today with just -M pc and then update it to a newer QEMU version, your existing command line would no longer work because you then have s->osk && s->use_host_osk set. How about we make the ordering absolutely clear? If you define s->osk, that's the one we use. Fallback is either host (default on new instance types) or dummy (default for old instance types). That way we only break existing VM definitions without modeled machine version that use -device applesmc without passing in an OSK. That's a user base I'm ok breaking. if (s->osk) { /* Use osk. Fail hard if invalid (ie wrong length) */ } else if (s->fallback_to_host_osk) { /* Use host OSK. Fail hard if we can't find it */ } else { /* Old dummy key behavior */ } In addition, new -M pc and -M q35 definitions set applesmc.fallback-to-host-osk=on by default. Alex ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-10-26 10:57 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-22 16:14 [PATCH v4] isa-applesmc: provide OSK forwarding on Apple hosts Vladislav Yaroshchuk 2021-10-25 10:13 ` Alexander Graf 2021-10-25 14:14 ` Vladislav Yaroshchuk 2021-10-25 14:22 ` Daniel P. Berrangé 2021-10-25 14:42 ` Alexander Graf 2021-10-25 14:47 ` Daniel P. Berrangé 2021-10-25 14:53 ` Alexander Graf 2021-10-25 15:10 ` Daniel P. Berrangé 2021-10-25 19:45 ` Alexander Graf 2021-10-26 8:42 ` Daniel P. Berrangé 2021-10-26 10:41 ` Alexander Graf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).