Re: [PATCH] hw/misc: applesmc: use host osk as default on macs

[Phil Dennis-Jordan]

Apologies for the late reply, I've been rather busy…

Generally, I think this is an awesome feature addition. (I also agree with Paolo's modification.) A few additional concerns though:

1. Licensing

Given that the code it's heavily based on is copyright Apple Computer Inc., licensed under APSL, is it safe including it in qemu as is?

If the integrated code is going to be quite so "directly inspired" (down to the inconsistent struct definition style and mixing unrelated constants in the same anonymous enum), perhaps at minimum place it in its own isolated source file with appropriate notice?

2. The actual code seems somewhat more verbose/generic than it needs to be, from my point of view. For example:

- IOConnectCallMethod calls with kSMCUserClientOpen/kSMCUserClientClose seem unnecessarily verbose. Using IOConnectCallStructMethod or IOConnectCallScalarMethod would be a little more compact. Or you could leave them out entirely, because the code still works if they're missing.

- There's error handling for cases that can't ever happen in the code as it stands, such as:   if (key == 0 || bytes == NULL) {

- There's distinct error handling for different kinds of failure modes in the helper function, percolating IOReturn codes to the caller, but the way the helper function is called is effectively just a boolean success/failure, so why bother with the complexity?

- The connection to the AppleSMC service is closed and reopened between reading the OSK0 and OSK1 keys. This isn't necessary. (The way the whole thing flows only really makes sense if you treat OSK0/OSK1 as 2 of many other 1-off keys, but I don't think the others are ever likely to be used in the same way in qemu as they'd constitute a VM escape.) 

I realise each of these aspects has essentially survived 1:1 from Apple's original code, which in my opinion would appear to make my first concern all the more worrying…

I'm not a Qemu maintainer though, so I'll leave it to actual maintainers to decide how much of a problem any of this is.

On Sat, 2 Oct 2021 at 22:24, Pedro Tôrres <t0rr3sp3dr0@gmail.com> wrote:

When running on a Mac, QEMU is able to get the host OSK and use it as
the default value for the AppleSMC device. The OSK query operation
doesn't require administrator privileges and can be executed by any user
on the system. This patch is based on open-source code from Apple, just
like the implementation from VirtualBox.

Apple:
https://opensource.apple.com/source/IOKitUser/IOKitUser-647.6.13/pwr_mgt.subproj/IOPMLibPrivate.c
https://opensource.apple.com/source/PowerManagement/PowerManagement-637.60.1/pmconfigd/PrivateLib.c

VirtualBox:
https://www.virtualbox.org/browser/vbox/trunk/src/VBox/Devices/EFI/DevSmc.cpp#L516

Signed-off-by: Pedro Tôrres <t0rr3sp3dr0@gmail.com>
---
hw/misc/applesmc.c | 185 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 185 insertions(+)

diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
index 1b9acaf..824135d 100644
--- a/hw/misc/applesmc.c
+++ b/hw/misc/applesmc.c
@@ -38,6 +38,171 @@
#include "qemu/timer.h"
#include "qom/object.h"

+#if defined(__APPLE__) && defined(__MACH__)
+#include <IOKit/IOKitLib.h>
+
+enum {
+    kSMCSuccess     = 0x00,
+    kSMCKeyNotFound = 0x84
+};
+
+enum {
+    kSMCUserClientOpen  = 0x00,
+    kSMCUserClientClose = 0x01,
+    kSMCHandleYPCEvent  = 0x02,
+    kSMCReadKey         = 0x05,
+    kSMCGetKeyInfo      = 0x09
+};
+
+typedef struct SMCVersion {
+    uint8_t  major;
+    uint8_t  minor;
+    uint8_t  build;
+    uint8_t  reserved;
+    uint16_t release;
+} SMCVersion;
+
+typedef struct SMCPLimitData {
+    uint16_t version;
+    uint16_t length;
+    uint32_t cpuPLimit;
+    uint32_t gpuPLimit;
+    uint32_t memPLimit;
+} SMCPLimitData;
+
+typedef struct SMCKeyInfoData {
+    IOByteCount dataSize;
+    uint32_t    dataType;
+    uint8_t     dataAttributes;
+} SMCKeyInfoData;
+
+typedef struct {
+    uint32_t       key;
+    SMCVersion     vers;
+    SMCPLimitData  pLimitData;
+    SMCKeyInfoData keyInfo;
+    uint8_t        result;
+    uint8_t        status;
+    uint8_t        data8;
+    uint32_t       data32;
+    uint8_t        bytes[32];
+} SMCParamStruct;
+
+static IOReturn smc_call_struct_method(uint32_t selector,
+                                       SMCParamStruct *inputStruct,
+                                       SMCParamStruct *outputStruct)
+{
+    IOReturn ret;
+
+    size_t inputStructCnt = sizeof(SMCParamStruct);
+    size_t outputStructCnt = sizeof(SMCParamStruct);
+
+    io_service_t smcService = IO_OBJECT_NULL;
+    io_connect_t smcConnect = IO_OBJECT_NULL;
+
+    smcService = IOServiceGetMatchingService(kIOMasterPortDefault,
+                                             IOServiceMatching("AppleSMC"));
+    if (smcService == IO_OBJECT_NULL) {
+        ret = kIOReturnNotFound;
+        goto exit;
+    }
+
+    ret = IOServiceOpen(smcService, mach_task_self(), 1, &smcConnect);
+    if (ret != kIOReturnSuccess) {
+        smcConnect = IO_OBJECT_NULL;
+        goto exit;
+    }
+    if (smcConnect == IO_OBJECT_NULL) {
+        ret = kIOReturnError;
+        goto exit;
+    }
+
+    ret = IOConnectCallMethod(smcConnect, kSMCUserClientOpen,
+                              NULL, 0, NULL, 0,
+                              NULL, NULL, NULL, NULL);
+    if (ret != kIOReturnSuccess) {
+        goto exit;
+    }
+
+    ret = IOConnectCallStructMethod(smcConnect, selector,
+                                    inputStruct, inputStructCnt,
+                                    outputStruct, &outputStructCnt);
+
+exit:
+    if (smcConnect != IO_OBJECT_NULL) {
+        IOConnectCallMethod(smcConnect, kSMCUserClientClose,
+                            NULL, 0, NULL, 0, NULL,
+                            NULL, NULL, NULL);
+        IOServiceClose(smcConnect);
+    }
+
+    return ret;
+}
+
+static IOReturn smc_read_key(uint32_t key,
+                             uint8_t *bytes,
+                             IOByteCount *dataSize)
+{
+    IOReturn ret;
+
+    SMCParamStruct inputStruct;
+    SMCParamStruct outputStruct;
+
+    if (key == 0 || bytes == NULL) {
+        ret = kIOReturnCannotWire;
+        goto exit;
+    }
+
+    /* determine key's data size */
+    memset(&inputStruct, 0, sizeof(SMCParamStruct));
+    inputStruct.data8 = kSMCGetKeyInfo;
+    inputStruct.key = key;
+
+    memset(&outputStruct, 0, sizeof(SMCParamStruct));
+    ret = smc_call_struct_method(kSMCHandleYPCEvent, &inputStruct, &outputStruct);
+    if (ret != kIOReturnSuccess) {
+        goto exit;
+    }
+    if (outputStruct.result == kSMCKeyNotFound) {
+        ret = kIOReturnNotFound;
+        goto exit;
+    }
+    if (outputStruct.result != kSMCSuccess) {
+        ret = kIOReturnInternalError;
+        goto exit;
+    }
+
+    /* get key value */
+    memset(&inputStruct, 0, sizeof(SMCParamStruct));
+    inputStruct.data8 = kSMCReadKey;
+    inputStruct.key = key;
+    inputStruct.keyInfo.dataSize = outputStruct.keyInfo.dataSize;
+
+    memset(&outputStruct, 0, sizeof(SMCParamStruct));
+    ret = smc_call_struct_method(kSMCHandleYPCEvent, &inputStruct, &outputStruct);
+    if (ret != kIOReturnSuccess) {
+        goto exit;
+    }
+    if (outputStruct.result == kSMCKeyNotFound) {
+        ret = kIOReturnNotFound;
+        goto exit;
+    }
+    if (outputStruct.result != kSMCSuccess) {
+        ret = kIOReturnInternalError;
+        goto exit;
+    }
+
+    memset(bytes, 0, *dataSize);
+    if (*dataSize > inputStruct.keyInfo.dataSize) {
+        *dataSize = inputStruct.keyInfo.dataSize;
+    }
+    memcpy(bytes, outputStruct.bytes, *dataSize);
+
+exit:
+    return ret;
+}
+#endif
+
/* #define DEBUG_SMC */

#define APPLESMC_DEFAULT_IOBASE        0x300
@@ -332,7 +497,27 @@ static void applesmc_isa_realize(DeviceState *dev, Error **errp)
                        s->iobase + APPLESMC_ERR_PORT);

    if (!s->osk || (strlen(s->osk) != 64)) {
+#if defined(__APPLE__) && defined(__MACH__)
+        IOReturn ret;
+        IOByteCount size = 32;
+
+        ret = smc_read_key('OSK0', (uint8_t *) default_osk, &size);
+        if (ret != kIOReturnSuccess) {
+            goto failure;
+        }
+
+        ret = smc_read_key('OSK1', (uint8_t *) default_osk + size, &size);
+        if (ret != kIOReturnSuccess) {
+            goto failure;
+        }
+
+        warn_report("Using AppleSMC with host key");
+        goto success;
+#endif
+failure:
        warn_report("Using AppleSMC with invalid key");
+
+success:
        s->osk = default_osk;
    }

--
2.30.1 (Apple Git-130)