From e0721534748f45daf0e41c24c368494c10fb787d Mon Sep 17 00:00:00 2001 From: Stan Rokita Date: Thu, 17 Oct 2019 11:58:02 -0700 Subject: [PATCH] MH2 | Check that subhal index is in range For each HalProxy method that takes in a sensorHandle, the code now checks that the subhal index byte (first byte) of the handle is the range of subhals in list before indexing into mSubHalList so that are crash does not occur and returns Result::BAD_VALUE if that is the case. Add unit tests to assert that error is returned. Also change the methods that accept sensorHandles to int32_t type since that is the actual type defined in the HAL spec. Bug: 136511617 Test: Passing new unit tests. Change-Id: I5489e999bc5eaef1a21698bdbc0a0341bc88194c --- sensors/2.0/multihal/HalProxy.cpp | 43 ++++++++++++++++---- sensors/2.0/multihal/include/HalProxy.h | 23 +++++++---- sensors/2.0/multihal/tests/HalProxy_test.cpp | 29 +++++++++++++ 3 files changed, 81 insertions(+), 14 deletions(-) diff --git a/sensors/2.0/multihal/HalProxy.cpp b/sensors/2.0/multihal/HalProxy.cpp index fe2b98b21a..38a627fbec 100644 --- a/sensors/2.0/multihal/HalProxy.cpp +++ b/sensors/2.0/multihal/HalProxy.cpp @@ -42,6 +42,8 @@ using ::android::hardware::sensors::V2_0::implementation::kWakelockTimeoutNs; typedef ISensorsSubHal*(SensorsHalGetSubHalFunc)(uint32_t*); +static constexpr int32_t kBitsAfterSubHalIndex = 24; + /** * Set the subhal index as first byte of sensor handle and return this modified version. * @@ -50,8 +52,19 @@ typedef ISensorsSubHal*(SensorsHalGetSubHalFunc)(uint32_t*); * * @return The modified sensor handle. */ -uint32_t setSubHalIndex(uint32_t sensorHandle, size_t subHalIndex) { - return sensorHandle | (subHalIndex << 24); +int32_t setSubHalIndex(int32_t sensorHandle, size_t subHalIndex) { + return sensorHandle | (static_cast(subHalIndex) << kBitsAfterSubHalIndex); +} + +/** + * Extract the subHalIndex from sensorHandle. + * + * @param sensorHandle The sensorHandle to extract from. + * + * @return The subhal index. + */ +size_t extractSubHalIndex(int32_t sensorHandle) { + return static_cast(sensorHandle >> kBitsAfterSubHalIndex); } HalProxy::HalProxy() { @@ -101,6 +114,9 @@ Return HalProxy::setOperationMode(OperationMode mode) { } Return HalProxy::activate(int32_t sensorHandle, bool enabled) { + if (!isSubHalIndexValid(sensorHandle)) { + return Result::BAD_VALUE; + } return getSubHalForSensorHandle(sensorHandle) ->activate(clearSubHalIndex(sensorHandle), enabled); } @@ -174,11 +190,17 @@ Return HalProxy::initialize( Return HalProxy::batch(int32_t sensorHandle, int64_t samplingPeriodNs, int64_t maxReportLatencyNs) { + if (!isSubHalIndexValid(sensorHandle)) { + return Result::BAD_VALUE; + } return getSubHalForSensorHandle(sensorHandle) ->batch(clearSubHalIndex(sensorHandle), samplingPeriodNs, maxReportLatencyNs); } Return HalProxy::flush(int32_t sensorHandle) { + if (!isSubHalIndexValid(sensorHandle)) { + return Result::BAD_VALUE; + } return getSubHalForSensorHandle(sensorHandle)->flush(clearSubHalIndex(sensorHandle)); } @@ -192,6 +214,9 @@ Return HalProxy::injectSensorData(const Event& event) { } if (result == Result::OK) { Event subHalEvent = event; + if (!isSubHalIndexValid(event.sensorHandle)) { + return Result::BAD_VALUE; + } subHalEvent.sensorHandle = clearSubHalIndex(event.sensorHandle); result = getSubHalForSensorHandle(event.sensorHandle)->injectSensorData(subHalEvent); } @@ -326,7 +351,7 @@ void HalProxy::initializeSensorList() { ALOGE("SubHal sensorHandle's first byte was not 0"); } else { ALOGV("Loaded sensor: %s", sensor.name.c_str()); - sensor.sensorHandle |= (subHalIndex << 24); + sensor.sensorHandle = setSubHalIndex(sensor.sensorHandle, subHalIndex); setDirectChannelFlags(&sensor, subHal); mSensors[sensor.sensorHandle] = sensor; } @@ -539,8 +564,12 @@ void HalProxy::setDirectChannelFlags(SensorInfo* sensorInfo, ISensorsSubHal* sub } } -ISensorsSubHal* HalProxy::getSubHalForSensorHandle(uint32_t sensorHandle) { - return mSubHalList[static_cast(sensorHandle >> 24)]; +ISensorsSubHal* HalProxy::getSubHalForSensorHandle(int32_t sensorHandle) { + return mSubHalList[extractSubHalIndex(sensorHandle)]; +} + +bool HalProxy::isSubHalIndexValid(int32_t sensorHandle) { + return extractSubHalIndex(sensorHandle) < mSubHalList.size(); } size_t HalProxy::countNumWakeupEvents(const std::vector& events, size_t n) { @@ -554,11 +583,11 @@ size_t HalProxy::countNumWakeupEvents(const std::vector& events, size_t n return numWakeupEvents; } -uint32_t HalProxy::clearSubHalIndex(uint32_t sensorHandle) { +int32_t HalProxy::clearSubHalIndex(int32_t sensorHandle) { return sensorHandle & (~kSensorHandleSubHalIndexMask); } -bool HalProxy::subHalIndexIsClear(uint32_t sensorHandle) { +bool HalProxy::subHalIndexIsClear(int32_t sensorHandle) { return (sensorHandle & kSensorHandleSubHalIndexMask) == 0; } diff --git a/sensors/2.0/multihal/include/HalProxy.h b/sensors/2.0/multihal/include/HalProxy.h index 26bc6447a0..10fce43ebb 100644 --- a/sensors/2.0/multihal/include/HalProxy.h +++ b/sensors/2.0/multihal/include/HalProxy.h @@ -124,7 +124,7 @@ class HalProxy : public ISensors, public IScopedWakelockRefCounter { * * @return The sensor info object in the mapping. */ - const SensorInfo& getSensorInfo(uint32_t sensorHandle) { return mSensors[sensorHandle]; } + const SensorInfo& getSensorInfo(int32_t sensorHandle) { return mSensors[sensorHandle]; } bool areThreadsRunning() { return mThreadsRun.load(); } @@ -177,10 +177,10 @@ class HalProxy : public ISensors, public IScopedWakelockRefCounter { * The subhal index is encoded in the first byte of the sensor handle and the remaining * bytes are generated by the subhal to identify the sensor. */ - std::map mSensors; + std::map mSensors; //! Map of the dynamic sensors that have been added to halproxy. - std::map mDynamicSensors; + std::map mDynamicSensors; //! The current operation mode for all subhals. OperationMode mCurrentOperationMode = OperationMode::NORMAL; @@ -192,7 +192,7 @@ class HalProxy : public ISensors, public IScopedWakelockRefCounter { static const int64_t kPendingWriteTimeoutNs = 5 * INT64_C(1000000000) /* 5 seconds */; //! The bit mask used to get the subhal index from a sensor handle. - static constexpr uint32_t kSensorHandleSubHalIndexMask = 0xFF000000; + static constexpr int32_t kSensorHandleSubHalIndexMask = 0xFF000000; /** * A FIFO queue of pairs of vector of events and the number of wakeup events in that vector @@ -319,7 +319,16 @@ class HalProxy : public ISensors, public IScopedWakelockRefCounter { * * @param sensorHandle The handle used to identify a sensor in one of the subhals. */ - ISensorsSubHal* getSubHalForSensorHandle(uint32_t sensorHandle); + ISensorsSubHal* getSubHalForSensorHandle(int32_t sensorHandle); + + /** + * Checks that sensorHandle's subhal index byte is within bounds of mSubHalList. + * + * @param sensorHandle The sensor handle to check. + * + * @return true if sensorHandles's subhal index byte is valid. + */ + bool isSubHalIndexValid(int32_t sensorHandle); /** * Count the number of wakeup events in the first n events of the vector. @@ -338,14 +347,14 @@ class HalProxy : public ISensors, public IScopedWakelockRefCounter { * * @return The modified version of the sensor handle. */ - static uint32_t clearSubHalIndex(uint32_t sensorHandle); + static int32_t clearSubHalIndex(int32_t sensorHandle); /** * @param sensorHandle The sensor handle to modify. * * @return true if subHalIndex byte of sensorHandle is zeroed. */ - static bool subHalIndexIsClear(uint32_t sensorHandle); + static bool subHalIndexIsClear(int32_t sensorHandle); }; /** diff --git a/sensors/2.0/multihal/tests/HalProxy_test.cpp b/sensors/2.0/multihal/tests/HalProxy_test.cpp index 040e8c2ca4..75a4c22d11 100644 --- a/sensors/2.0/multihal/tests/HalProxy_test.cpp +++ b/sensors/2.0/multihal/tests/HalProxy_test.cpp @@ -663,6 +663,35 @@ TEST(HalProxyTest, DynamicSensorsDisconnectedTest) { } } +TEST(HalProxyTest, InvalidSensorHandleSubHalIndexProxyCalls) { + constexpr size_t kNumSubHals = 3; + constexpr size_t kQueueSize = 5; + int32_t kNumSubHalsInt32 = static_cast(kNumSubHals); + std::vector subHalObjs(kNumSubHals); + std::vector subHals; + for (const auto& subHal : subHalObjs) { + subHals.push_back((ISensorsSubHal*)(&subHal)); + } + + std::unique_ptr eventQueue = makeEventFMQ(kQueueSize); + std::unique_ptr wakeLockQueue = makeWakelockFMQ(kQueueSize); + ::android::sp callback = new SensorsCallback(); + HalProxy proxy(subHals); + // Initialize for the injectSensorData call so callback postEvents is valid + proxy.initialize(*eventQueue->getDesc(), *wakeLockQueue->getDesc(), callback); + + // For testing proxy.injectSensorData properly + proxy.setOperationMode(OperationMode::DATA_INJECTION); + + // kNumSubHalsInt32 index is one off the end of mSubHalList in proxy object + EXPECT_EQ(proxy.activate(0x00000001 | (kNumSubHalsInt32 << 24), true), Result::BAD_VALUE); + EXPECT_EQ(proxy.batch(0x00000001 | (kNumSubHalsInt32 << 24), 0, 0), Result::BAD_VALUE); + EXPECT_EQ(proxy.flush(0x00000001 | (kNumSubHalsInt32 << 24)), Result::BAD_VALUE); + Event event; + event.sensorHandle = 0x00000001 | (kNumSubHalsInt32 << 24); + EXPECT_EQ(proxy.injectSensorData(event), Result::BAD_VALUE); +} + // Helper implementations follow void testSensorsListFromProxyAndSubHal(const std::vector& proxySensorsList, const std::vector& subHalSensorsList) {