Fix SMC reading values #8583
Fix SMC reading values #8583
Conversation
| #include <osquery/logger/logger.h> | ||
|
|
||
| #include <boost/algorithm/hex.hpp> | ||
| #include <boost/algorithm/string.hpp> |
There was a problem hiding this comment.
for string trimming
| inline uint64_t strtoull(const char* str, size_t size, size_t base) { | ||
| uint64_t total = 0; | ||
| for (size_t i = 0; i < size; i++) { | ||
| total += static_cast<uint64_t>(static_cast<unsigned char>(str[i])) | ||
| << (size - 1 - i) * 8; | ||
| } | ||
| return total; | ||
| } |
There was a problem hiding this comment.
Companion to strtoul, but for 64 bit ints.
There was a problem hiding this comment.
Other parts of our code use std::strtoull. Should this?
There was a problem hiding this comment.
I don't think so. The std:strtoull function does what you'd probably expect from the name: it takes a numeric string and interprets it as a number, e.g. the string "12345" becomes the unsigned long 12345. The strtoul function defined in smc_keys.cpp does something else entirely: it treats a string as a set of bytes representing an integer with big-endian encoding. It's functionally the same as byte-swapping a string and memcpying the bytes into a new uint32_t, except it would work equally well on machines that actually use big-endian encoding (I don't think that was a concern even nine years ago when this code was written, but 🤷 ).
I don't know why the function here was called strtoul, but for consistency I named the 64-bit version the same thing. I'd be happy to rename them both, but without knowing the history I prefer to stay low-touch.
There was a problem hiding this comment.
I agreee consistency with the local function makes sense, though the divergence from std:: seems unfortunate.
| return total; | ||
| } | ||
|
|
||
| double getConvertedValue(const std::string& smcType, |
There was a problem hiding this comment.
Changed this (and all related vars) from float to double to handle 64-bit types. We don't utilize any in tables right now, but there are definitely some 64-bit SMC keys (18 on my system) and this future-proofs us.
931df9b to
77218cb
Compare
directionless
left a comment
There was a problem hiding this comment.
nice! A couple of small questions...
| inline uint64_t strtoull(const char* str, size_t size, size_t base) { | ||
| uint64_t total = 0; | ||
| for (size_t i = 0; i < size; i++) { | ||
| total += static_cast<uint64_t>(static_cast<unsigned char>(str[i])) | ||
| << (size - 1 - i) * 8; | ||
| } | ||
| return total; | ||
| } |
There was a problem hiding this comment.
Other parts of our code use std::strtoull. Should this?
| else if ((type[0] == 's' || type[0] == 'u') && type[1] == 'i') { | ||
| uint64_t intVal = strtoull(val.c_str(), size, 16); | ||
| if (type[0] == 's') { |
There was a problem hiding this comment.
Why combine the s and u logic if it's just split apart on line 462?
There was a problem hiding this comment.
It's combining the si and ui logic, both of which start by interpreting the value as a big-endian int. What we do with that int depends on whether it's signed or unsigned.
There was a problem hiding this comment.
Yeah.. I guess I'd duplicate the strtoull over the nested if But 🤷 I'll merge as is
| inline uint64_t strtoull(const char* str, size_t size, size_t base) { | ||
| uint64_t total = 0; | ||
| for (size_t i = 0; i < size; i++) { | ||
| total += static_cast<uint64_t>(static_cast<unsigned char>(str[i])) | ||
| << (size - 1 - i) * 8; | ||
| } | ||
| return total; | ||
| } |
There was a problem hiding this comment.
I agreee consistency with the local function makes sense, though the divergence from std:: seems unfortunate.
| else if ((type[0] == 's' || type[0] == 'u') && type[1] == 'i') { | ||
| uint64_t intVal = strtoull(val.c_str(), size, 16); | ||
| if (type[0] == 's') { |
There was a problem hiding this comment.
Yeah.. I guess I'd duplicate the strtoull over the nested if But 🤷 I'll merge as is
Fixes #7627
Fixes #5896
Fixes fleetdm/fleet#26583
Fixes issues with SMC key readings used by the
temperature_sensors,power_sensorsandfan_speed_sensorstables.SMC keys are already mysterious (there is no known definitive reference on the keys) and each new Apple product that comes out seems to have its own opinions about what data type keys should use. At some point the temperature keys started using the
fltandiofttypes which we were not handling.Big shout out to @directionless for doing some digging on a previous ticket and leading me to https://github.com/dkorunic/iSMC which at least has a good listing of known types and clues as to how to convert them. I've updated the code to handle
fltandioftas well all of the knownfp*,sp*,ui*andsi*types. I also added tests for the conversions.Before:
After:
