macOS: power_sensors table shows incorrect data

[sharvilshah]

Bug report

power_sensors table returns -1.00 value for all power related SMC keys, instead of actual values.

osquery> .all power_sensors;
+------+----------+-------------------+-------+
| key  | category | name              | value |
+------+----------+-------------------+-------+
| PBLC | power    | Battery Rail      | -1.00 |
| PC0R | power    | Mainboard S0 Rail | -1.00 |
| PCPC | power    | CPU Cores         | -1.00 |
| PCPG | power    | CPU GFX           | -1.00 |
| PDTR | power    | DC In Total       | -1.00 |
| PG0R | power    | GPU Rail          | -1.00 |
| PSTR | power    | System Total      | -1.00 |
| IC0R | current  | CPU Rail          | -1.00 |
| ID0R | current  | Mainboard S0 Rail | -1.00 |
| IG0C | current  | GPU Rail          | -1.00 |
| IM0C | current  | Memory Controller | -1.00 |
| IPBR | current  | Charger BMON      | -1.00 |
| VD0R | voltage  | Mainboard S0 Rail | -1.00 |
| VG0C | voltage  | GPU Core          | -1.00 |
| VP0R | voltage  | 12V Rail          | -1.00 |
+------+----------+-------------------+-------+

Joining with smc_keys table, we can see that the raw SMC data is still there:

osquery> select power_sensors.key,
    ...> category,
    ...> name,
    ...> power_sensors.value as power_value,
    ...> smc_keys.value as raw_smc_value,
    ...> smc_keys.type as smc_type,
    ...> smc_keys.size as smc_size
    ...> FROM power_sensors
    ...> JOIN smc_keys where power_sensors.key = smc_keys.key;
+------+----------+-------------------+-------------+---------------+----------+----------+
| key  | category | name              | power_value | raw_smc_value | smc_type | smc_size |
+------+----------+-------------------+-------------+---------------+----------+----------+
| PBLC | power    | Battery Rail      | -1.00       | 00000000      | flt      | 4        |
| PC0R | power    | Mainboard S0 Rail | -1.00       | 1f3ea240      | flt      | 4        |
| PCPC | power    | CPU Cores         | -1.00       | 0107          | sp87     | 2        |
| PCPG | power    | CPU GFX           | -1.00       | 0000          | sp87     | 2        |
| PDTR | power    | DC In Total       | -1.00       | 00000000      | flt      | 4        |
| PG0R | power    | GPU Rail          | -1.00       | 94fb7040      | flt      | 4        |
| PSTR | power    | System Total      | -1.00       | 06f9          | sp87     | 2        |
| IC0R | current  | CPU Rail          | -1.00       | 91adc13e      | flt      | 4        |
| ID0R | current  | Mainboard S0 Rail | -1.00       | ff9fb939      | flt      | 4        |
| IG0C | current  | GPU Rail          | -1.00       | 00000000      | flt      | 4        |
| IM0C | current  | Memory Controller | -1.00       | 00000000      | flt      | 4        |
| IPBR | current  | Charger BMON      | -1.00       | 4d75983f      | flt      | 4        |
| VD0R | voltage  | Mainboard S0 Rail | -1.00       | 00000000      | flt      | 4        |
| VG0C | voltage  | GPU Core          | -1.00       | a5d54d3f      | flt      | 4        |
| VP0R | voltage  | 12V Rail          | -1.00       | 164c4641      | flt      | 4        |
+------+----------+-------------------+-------------+---------------+----------+----------+

This suggests that we may not be converting the various SMC data types correctly. I will dig deeper on this later.

What operating system and version are you using?

osquery> SELECT version, build, platform FROM os_version;
 version = 10.13.6
   build = 17G8037
platform = darwin

This is seen on 10.13, 10.14 and 10.15

What version of osquery are you using?

osquery> SELECT version from osquery_info;
version = 4.0.2-44-g6ba37014

[directionless]

Reading this, it seems weird to have this both in power_sensors and smc_keys.

[sharvilshah]

@directionless previous discussion regarding that here: https://github.com/osquery/osquery/issues/1854

[directionless]

Ah. Thanks for the context.

Yeah, it looks like we're missing types -- https://github.com/osquery/osquery/blob/4f78848794cd2df1532b9c8d7e3fe4b17dde3e2a/osquery/tables/system/darwin/smc_keys.cpp#L381-L404

Probably a good thing to add to CI

[theopolis]

Any progress on this? I look a quick look and I'm not sure how to convert the hex values to floats (is there a common pattern for this?)

[sharvilshah]

I think so...the spXY values are always 2 bytes and it seems to be signed fixed point, where the MSB is the sign bit followed by X integer bits and Y fractional bits.

sp87 would be S I I I I I I I I F F F F F F F I think. Need to try it out though.

Not sure how flt is represented. Perhaps single precision float?

[directionless]

I dug at this s bit over a month ago. I did not find official docs, but I did find several implementations that had pretty clean parsers for this. I saw implementations based on lookup table, as well as a try repeatedly model.

I did not get to testing any of this, so it's something closer to a linkdump:

• https://github.com/beltex/libsmc
• https://github.com/dzitkowskik/osx-system-data-logger/blob/master/osx-system-data-logger/smc.cpp
• https://github.com/dkorunic/iSMC/blob/52bb59e0c43fedfeea5511ee9f541c0e09142c5a/smc/conv.go
• https://github.com/acidanthera/VirtualSMC/blob/master/Docs/SMCKeys.txt
• https://github.com/kzlekk/HWSensors/blob/ddf53ec17864fec389ce6e51a5f7385c99e17673/Shared/SmcHelper.m

[Smjert]

Stumbled upon this today (mostly due to a user asking on Slack).

From the quick look, I could see that we are missing the flt type and that we are sometimes incorrectly comparing the type string, because it seems that all types are 4 bytes padded with spaces, so if the type is 3 characters, the last is a space. We are comparing ui8 with ui8, so that comparison fails, and at least in the smc_keys table we display the values as hex. A similar problem would exist for flt. Also a flt value is just a 4 byte float when unhexed, so a memcpy into a float type seems to be enough.

EDIT: I also haven't fully analyzed the code but it seems that in power_sensors we are using the values of the SMC keys that we convert to string for smc_keys; while using another table to make another function is not always ideal, this case should be just a case of direct copying the values, but it seems that for smc_keys we don't support decoding the same set of types than power_sensors, so there are some values that we have to unhex. I think the whole logic should be shared, meaning, smc_keys is the table that gets the support for all types, and then power_values just takes the correct rows that match the key it's interested in, and direct copy the output string of the column, with no conversion in between.