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

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

[Pedro Tôrres]

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)

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

[Paolo Bonzini]

On 02/10/21 22:24, Pedro Tôrres wrote:
> #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;
>      }
> 
> -- 

I think it is incorrect to use host key if strlen(s->osk) != 64.  So I
would change this code to something like this:

@@ -315,6 +480,7 @@ static const MemoryRegionOps applesmc_err_io_ops = {
  static void applesmc_isa_realize(DeviceState *dev, Error **errp)
  {
      AppleSMCState *s = APPLE_SMC(dev);
+    bool valid_key = false;
  
      memory_region_init_io(&s->io_data, OBJECT(s), &applesmc_data_io_ops, s,
                            "applesmc-data", 1);
@@ -331,7 +497,31 @@ 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->osk || (strlen(s->osk) != 64)) {
+    if (s->osk) {
+        valid_key = strlen(s->osk) == 64;
+    } else {
+#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");
+        valid_key = true;
+        s->osk = default_osk;
+failure:
+#endif
+    }
+
+    if (!valid_key) {
          warn_report("Using AppleSMC with invalid key");
          s->osk = default_osk;
      }

Otherwise looks good, so I queued it (haven't yet compile-tested it, but I
will before sending out my pull request).

Thanks,

Paolo

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

[Phil Dennis-Jordan]

[-- Attachment #1: Type: text/plain, Size: 9240 bytes --]

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)
>
>

[-- Attachment #2: Type: text/html, Size: 11498 bytes --]

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

[Paolo Bonzini]

On 08/10/21 14:03, Phil Dennis-Jordan wrote:
> 
> 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?

Yeah, this should be reverted.

Pedro, I understand that you stated it was "based on code from Apple" 
but you also said (by including Signed-off-by) that

---
(a) The contribution was created in whole or in part by me and I
     have the right to submit it under the open source license
     indicated in the file; or

(b) The contribution is based upon previous work that, to the best
     of my knowledge, is covered under an appropriate open source
     license and I have the right under that license to submit that
     work with modifications, whether created in whole or in part
     by me, under the same open source license (unless I am
     permitted to submit under a different license), as indicated
     in the file; or
---

and this is not true.

Thanks very much,

Paolo

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

[Pedro Tôrres]

[-- Attachment #1: Type: text/plain, Size: 1679 bytes --]

Hey Paolo and Phil,

I understand you concerns regarding the license that Apple open-source code is distributed.

If I restart from scratch and implement this feature based only on VirtualBox code, that is distributed under GPLv2, would it be fine?

Best regards,
Pedro Tôrres

> On Oct 8, 2021, at 3:54 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 08/10/21 14:03, Phil Dennis-Jordan wrote:
>> 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?
> 
> Yeah, this should be reverted.
> 
> Pedro, I understand that you stated it was "based on code from Apple" but you also said (by including Signed-off-by) that
> 
> ---
> (a) The contribution was created in whole or in part by me and I
>    have the right to submit it under the open source license
>    indicated in the file; or
> 
> (b) The contribution is based upon previous work that, to the best
>    of my knowledge, is covered under an appropriate open source
>    license and I have the right under that license to submit that
>    work with modifications, whether created in whole or in part
>    by me, under the same open source license (unless I am
>    permitted to submit under a different license), as indicated
>    in the file; or
> ---
> 
> and this is not true.
> 
> Thanks very much,
> 
> Paolo

[-- Attachment #2: Type: text/html, Size: 2800 bytes --]

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

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 1933 bytes --]

Can you instead provide documentation in English (pseudocode, tables of the
structs, etc.)? That's the safest bet.

Paolo

El sáb., 9 oct. 2021 7:32, Pedro Tôrres <t0rr3sp3dr0@gmail.com> escribió:

> Hey Paolo and Phil,
>
> I understand you concerns regarding the license that Apple open-source
> code is distributed.
>
> If I restart from scratch and implement this feature based only on
> VirtualBox code, that is distributed under GPLv2, would it be fine?
>
> Best regards,
> Pedro Tôrres
>
> On Oct 8, 2021, at 3:54 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 08/10/21 14:03, Phil Dennis-Jordan wrote:
>
> 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?
>
>
> Yeah, this should be reverted.
>
> Pedro, I understand that you stated it was "based on code from Apple" but
> you also said (by including Signed-off-by) that
>
> ---
> (a) The contribution was created in whole or in part by me and I
>    have the right to submit it under the open source license
>    indicated in the file; or
>
> (b) The contribution is based upon previous work that, to the best
>    of my knowledge, is covered under an appropriate open source
>    license and I have the right under that license to submit that
>    work with modifications, whether created in whole or in part
>    by me, under the same open source license (unless I am
>    permitted to submit under a different license), as indicated
>    in the file; or
> ---
>
> and this is not true.
>
> Thanks very much,
>
> Paolo
>
>

[-- Attachment #2: Type: text/html, Size: 3176 bytes --]

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

[Pedro Tôrres]

[-- Attachment #1: Type: text/plain, Size: 3163 bytes --]

AFAIK there’s no public documentation from Apple on how to read values from SMC.

But Amit Singh wrote a book, Mac OS X Internals, and one of the processes described on it is how to read OSK directly from SMC: https://web.archive.org/web/20190630050544/http://osxbook.com/book/bonus/chapter7/tpmdrmmyth/

This is actually referenced on VirtualBox’s source code as the base for their implementation: https://www.virtualbox.org/browser/vbox/trunk/src/VBox/Devices/EFI/DevSmc.cpp#L520

Additionally, there is the smcFanControl project, licensed under GPLv2, that performs reads and writes on SMC and have all information necessary to implement this feature on QEMU: https://github.com/hholtmann/smcFanControl

This project was used as base for the SMC in-tree driver for Linux and it’s referenced there: https://github.com/torvalds/linux/blob/master/drivers/hwmon/applesmc.c#L14

I think we would be safe using these sources as the base for our implementation, given that other huge GPL projects like Linux and VirtualBox have been using them for years.

Best regards,
Pedro Tôrres

> On Oct 10, 2021, at 4:25 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> Can you instead provide documentation in English (pseudocode, tables of the structs, etc.)? That's the safest bet.
> 
> Paolo
> 
> El sáb., 9 oct. 2021 7:32, Pedro Tôrres <t0rr3sp3dr0@gmail.com> escribió:
>> Hey Paolo and Phil,
>> 
>> I understand you concerns regarding the license that Apple open-source code is distributed.
>> 
>> If I restart from scratch and implement this feature based only on VirtualBox code, that is distributed under GPLv2, would it be fine?
>> 
>> Best regards,
>> Pedro Tôrres
>> 
>>> On Oct 8, 2021, at 3:54 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> On 08/10/21 14:03, Phil Dennis-Jordan wrote:
>>>> 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?
>>> 
>>> Yeah, this should be reverted.
>>> 
>>> Pedro, I understand that you stated it was "based on code from Apple" but you also said (by including Signed-off-by) that
>>> 
>>> ---
>>> (a) The contribution was created in whole or in part by me and I
>>>    have the right to submit it under the open source license
>>>    indicated in the file; or
>>> 
>>> (b) The contribution is based upon previous work that, to the best
>>>    of my knowledge, is covered under an appropriate open source
>>>    license and I have the right under that license to submit that
>>>    work with modifications, whether created in whole or in part
>>>    by me, under the same open source license (unless I am
>>>    permitted to submit under a different license), as indicated
>>>    in the file; or
>>> ---
>>> 
>>> and this is not true.
>>> 
>>> Thanks very much,
>>> 
>>> Paolo

[-- Attachment #2: Type: text/html, Size: 5367 bytes --]

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

[Gabriel L. Somlo]

FWIW, there's an applesmc driver in the Linux kernel, and it exposes
many of the keys and values stored on the chip under
/sys/devices/platform/applesmc.768 (or at least it *used* to back when
I last checked).

My idea at the time was to get this driver to also expose the value of
OSK0,1, so that userspace programs (like e.g. qemu) could read and use
the values hardcoded in hardware without needing to hardcode them
themselves in software.

It went nowhere at the time (the hwmon maintainer kept insisting that
"since it's a constant why don't you just hardcode it", and round and
round we went until I walked away:

https://lore.kernel.org/lkml/20121210222313.GF2097@hedwig.ini.cmu.edu/

Given *this* conversation, it might be worth someone's effort to try
that approach again. IMO it's really the most efficient: have an
already existing applesmc driver in the hypervisor's kernel expose the
desired key values (it's whole job is to expose key values to
userspace in the first place). Then have userspace read that and use
it for whatever purposes (including populating guest-facing emulated
smc devices). Nobody has to use anyone's copyrighted code or strings
or whatever. If only the hwmon folks could be convinced this time
around :)

HTH,
--Gabriel

On Sun, Oct 10, 2021 at 06:22:04PM -0300, Pedro Tôrres wrote:
> AFAIK there’s no public documentation from Apple on how to read values from
> SMC.
> 
> But Amit Singh wrote a book, Mac OS X Internals, and one of the processes
> described on it is how to read OSK directly from SMC: https://web.archive.org/
> web/20190630050544/http://osxbook.com/book/bonus/chapter7/tpmdrmmyth/
> 
> This is actually referenced on VirtualBox’s source code as the base for their
> implementation: https://www.virtualbox.org/browser/vbox/trunk/src/VBox/Devices/
> EFI/DevSmc.cpp#L520
> 
> Additionally, there is the smcFanControl project, licensed under GPLv2, that
> performs reads and writes on SMC and have all information necessary to
> implement this feature on QEMU: https://github.com/hholtmann/smcFanControl
> 
> This project was used as base for the SMC in-tree driver for Linux and it’s
> referenced there: https://github.com/torvalds/linux/blob/master/drivers/hwmon/
> applesmc.c#L14
> 
> I think we would be safe using these sources as the base for our
> implementation, given that other huge GPL projects like Linux and VirtualBox
> have been using them for years.
> 
> Best regards,
> Pedro Tôrres
> 
> 
>     On Oct 10, 2021, at 4:25 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> 
>     Can you instead provide documentation in English (pseudocode, tables of the
>     structs, etc.)? That's the safest bet.
> 
>     Paolo
> 
>     El sáb., 9 oct. 2021 7:32, Pedro Tôrres <t0rr3sp3dr0@gmail.com> escribió:
> 
>         Hey Paolo and Phil,
> 
>         I understand you concerns regarding the license that Apple open-source
>         code is distributed.
> 
>         If I restart from scratch and implement this feature based only on
>         VirtualBox code, that is distributed under GPLv2, would it be fine?
> 
>         Best regards,
>         Pedro Tôrres
> 
> 
>             On Oct 8, 2021, at 3:54 PM, Paolo Bonzini <pbonzini@redhat.com>
>             wrote:
> 
> 
>             On 08/10/21 14:03, Phil Dennis-Jordan wrote:
> 
>                 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?
> 
>            
>             Yeah, this should be reverted.
>            
>             Pedro, I understand that you stated it was "based on code from
>             Apple" but you also said (by including Signed-off-by) that
>            
>             ---
>             (a) The contribution was created in whole or in part by me and I
>                have the right to submit it under the open source license
>                indicated in the file; or
>            
>             (b) The contribution is based upon previous work that, to the best
>                of my knowledge, is covered under an appropriate open source
>                license and I have the right under that license to submit that
>                work with modifications, whether created in whole or in part
>                by me, under the same open source license (unless I am
>                permitted to submit under a different license), as indicated
>                in the file; or
>             ---
>            
>             and this is not true.
>            
>             Thanks very much,
>            
>             Paolo
>            
> 

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

[BALATON Zoltan]

On Mon, 11 Oct 2021, Gabriel L. Somlo wrote:
> FWIW, there's an applesmc driver in the Linux kernel, and it exposes
> many of the keys and values stored on the chip under
> /sys/devices/platform/applesmc.768 (or at least it *used* to back when
> I last checked).
>
> My idea at the time was to get this driver to also expose the value of
> OSK0,1, so that userspace programs (like e.g. qemu) could read and use
> the values hardcoded in hardware without needing to hardcode them
> themselves in software.

I guess a frequent use case for running macOS guests with keys from host 
would be on hosts running macOS too so a solution that works both on macOS 
and Linux might be better than a Linux specific one which then needs 
another way to do the same on macOS. Looks like there's free code for that 
too and you don't have to convince a maintainer either.

Regards,
BALATON Zoltan

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

[Gabriel L. Somlo]

On Mon, 11 Oct 2021 15:40:07 +0200, balaton@eik.bme.hu wrote:
> I guess a frequent use case for running macOS guests with keys from host 
> would be on hosts running macOS too so a solution that works both on macOS 
> and Linux might be better than a Linux specific one which then needs 
> another way to do the same on macOS. Looks like there's free code for that 
> too and you don't have to convince a maintainer either.

I mostly agree with you (hadn't given much thought to qemu on macOSX),
with the small caveat that (on Linux) you'll end up racing the kernel
applesmc driver for access to the physical hardware; not sure whether
you'd run into anything similar on host-side macOSX as well, never
actually used it much as a developer... :)

Cheers,
--Gabriel

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

[Phil Dennis-Jordan]

[-- Attachment #1: Type: text/plain, Size: 5222 bytes --]

On Mon, 11 Oct 2021 at 15:19, Gabriel L. Somlo <gsomlo@gmail.com> wrote:

> Given *this* conversation, it might be worth someone's effort to try
> that approach again. IMO it's really the most efficient: have an
> already existing applesmc driver in the hypervisor's kernel expose the
> desired key values (it's whole job is to expose key values to
> userspace in the first place). Then have userspace read that and use
> it for whatever purposes (including populating guest-facing emulated
> smc devices). Nobody has to use anyone's copyrighted code or strings
> or whatever. If only the hwmon folks could be convinced this time
> around :)
>

This is very similar to the situation on macOS. The way to request the
information from the kernel driver from userspace would of course be
different. (IOKitLib on macOS, sysfs or perhaps ioctls on some /dev node on
Linux.)

I can give a quick rundown on the situation on macOS:
- The SMC device is matched by the AppleSMC.kext kernel-mode driver.
- This driver provides a bunch of other functionality such the SMC's
hardware watchdog etc., but also the basics of reading these SMC keys.
- The driver offers a userspace-facing API via the "I/O Kit" HAL mechanism
that underpins almost all interfacing with device drivers on macOS.
- The IOKit node in question is "AppleSMC"; this service can be "opened"
(IOServiceOpen()) by user processes, with parameter type=0.
- With an open service connection, there are a number of different "method
calls" (via the IOConnectCall*Method() family of functions) that can be
invoked on the driver.

From here onwards you pretty much have to look at the source code that
Pedro linked in the original message, there's no actual documentation.

- One of these method calls (selector=2, Apple calls
this kSMCHandleYPCEvent) accepts a command struct (Apple calls
this SMCParamStruct) and returns a modified version of this struct. This
allows you to perform operations on each of the many defined "Keys" in the
SMC. (The keys are identified via 4 bytes which can be interpreted as 4
ASCII characters, like FourCC codes - GCC and clang support this by
multi-character character literals, e.g. 'OSK0'. The key is selected in the
first 4 bytes of the struct.)
- Within this struct there's a kind of command selector (data8 field, 1
byte, offset 42), one possible value is kSMCReadKey = 5 for reading the key
value. In that case you also need to provide space at the end of the output
struct (offset 48) for the read data and indicate in a size field (offset
28, uint32) how many bytes you're reading. (32 bytes seems to be the
maximum, which also coincides with the sizes of each of the OSK halves.
This means the struct is 80 bytes in total.) Everything else can be
0-initialised. There is a 1-byte "result" field at offset 20 which will be
0 if the call was successful. (And indeed the IOConnectCall*Method()
library function itself must return kIOReturnSuccess = 0 as well.)

To read the 2 OSK keys on macOS therefore, you need to perform 5 steps:
1. Find the AppleSMC IOKit service:
io_service_t service = IOServiceGetMatchingService(kIOMasterPortDefault,
IOServiceMatching("AppleSMC"));
2. Open a connection to the service (assuming it was found):
IOServiceOpen(service, mach_task_self(), 0, &connection);
3. Initialise the SMC command struct with key 'OSK0', command 5 (read key),
and set the data size to 32 for reading 32 bytes of data. Pass this as the
"input" struct argument to IOConnectCallStructMethod(connection, …) and a
pointer to another instance of such a struct as the output, and use method
selector 2.
4. Same for 'OSK1'
5. Clean up by closing the service connection (IOServiceClose) and
releasing the service handle. (IOObjectReturn)

Assuming appropriate error checking at every stage, the values for the 2
keys will be in the data fields of the respective output structs.

From this I *think* it should be possible to put together a working
implementation on macOS hosts. Pedro's original code did a lot more, but
anything outside of the above is essentially fluff.


Support for Linux would be great too; does the applesmc driver create a
node in /dev? If so, perhaps we can persuade the maintainers to accept a
patch with an ioctl for submitting commands directly to the SMC? Then it's
not specifically the OSK keys, but *any* key. The device supports more keys
than it publicly advertises, after all. (Such a feature would be useful for
improving qemu's virtual AppleSMC incidentally - there's a ~10 second boot
delay for macOS guests and I strongly suspect it's at least in part down to
the SMC not behaving as the OS expects. For example, the OS wants the
watchdog feature, but Qemu's virtual SMC does not provide it. If we could
easily submit arbitrary SMC commands to the physical device from a guest
running inside Qemu during development, we could work out some of that
hidden behaviour.)

Happy to answer any questions on the macOS side - for what it's worth, I'm
not affiliated with Apple in any way, but I do a lot of systems-level
development on their platforms and know the IOKit inside out.

Hope that helps!
Phil

[-- Attachment #2: Type: text/html, Size: 5988 bytes --]