HAL: Fixed vendor HAL crash due to use-after-free
Vendor HAL crash happened due to use-after-free during RSSI monitor operation.
So, segmentation fault happened due to wrong memory access.
The situation is as follows:
-----------------------
// 11-21 13:26:17.284209 827 827 I WifiHAL : Successfully set RSSI monitoring
1. RSSI monitoring start
// The object(SetRSSIMonitorCommand, 0xb400007d2ba53970) has been deleted somewhere without calling unregisterVendorhandler().
2. About 40mins passed ~
// cmd->addRef(); /* invalid memory access => addr 0x50455f5245544651 */
3. Hal crashed due to use-after-free
-----------------------
Any logs for unregisterVendorhandler() & freeing the SetRSSIMonitorCommand object doesn't exist during that time.
So, the object(SetRSSIMonitorCommand, 0xb400007d2ba53970) seems to have been freed for some unknown reason.
We decided to fix like below:
-----------------------
- Add a destructor on the class, run unregisterVendorhandler() when the object is destroyed.
- Add more debug logs to find out why the object is destroyed.
-----------------------
We expect that the HAL crash may not happen by the destructor operation.
Bug: 207297786
Test: Verified using halutil, rssi_monitor function works well.
Test: Passed sanity test
Change-Id: Iaa30c7e0d901ae65d807258bed8239951f9be942
diff --git a/bcmdhd/wifi_hal/common.cpp b/bcmdhd/wifi_hal/common.cpp
index 887b530..5a9d5ab 100755
--- a/bcmdhd/wifi_hal/common.cpp
+++ b/bcmdhd/wifi_hal/common.cpp
@@ -166,14 +166,11 @@
pthread_mutex_unlock(&info->cb_lock);
}
-void wifi_unregister_vendor_handler(wifi_handle handle, uint32_t id, int subcmd)
+void wifi_unregister_vendor_handler_without_lock(wifi_handle handle, uint32_t id, int subcmd)
{
hal_info *info = (hal_info *)handle;
- pthread_mutex_lock(&info->cb_lock);
-
for (int i = 0; i < info->num_event_cb; i++) {
-
if (info->event_cb[i].nl_cmd == NL80211_CMD_VENDOR
&& info->event_cb[i].vendor_id == id
&& info->event_cb[i].vendor_subcmd == subcmd) {
@@ -185,10 +182,16 @@
break;
}
}
-
- pthread_mutex_unlock(&info->cb_lock);
}
+void wifi_unregister_vendor_handler(wifi_handle handle, uint32_t id, int subcmd)
+{
+ hal_info *info = (hal_info *)handle;
+
+ pthread_mutex_lock(&info->cb_lock);
+ wifi_unregister_vendor_handler_without_lock(handle, id, subcmd);
+ pthread_mutex_unlock(&info->cb_lock);
+}
wifi_error wifi_register_cmd(wifi_handle handle, int id, WifiCommand *cmd)
{
diff --git a/bcmdhd/wifi_hal/common.h b/bcmdhd/wifi_hal/common.h
index d5bc971..9065802 100755
--- a/bcmdhd/wifi_hal/common.h
+++ b/bcmdhd/wifi_hal/common.h
@@ -424,6 +424,7 @@
uint32_t id, int subcmd, nl_recvmsg_msg_cb_t func, void *arg);
void wifi_unregister_handler(wifi_handle handle, int cmd);
+void wifi_unregister_vendor_handler_without_lock(wifi_handle handle, uint32_t id, int subcmd);
void wifi_unregister_vendor_handler(wifi_handle handle, uint32_t id, int subcmd);
wifi_error wifi_register_cmd(wifi_handle handle, int id, WifiCommand *cmd);
diff --git a/bcmdhd/wifi_hal/cpp_bindings.h b/bcmdhd/wifi_hal/cpp_bindings.h
index 456f31c..8b5600a 100755
--- a/bcmdhd/wifi_hal/cpp_bindings.h
+++ b/bcmdhd/wifi_hal/cpp_bindings.h
@@ -347,6 +347,10 @@
return wifi_register_vendor_handler(wifiHandle(), id, subcmd, &event_handler, this);
}
+ void unregisterVendorHandlerWithoutLock(uint32_t id, int subcmd) {
+ wifi_unregister_vendor_handler_without_lock(wifiHandle(), id, subcmd);
+ }
+
void unregisterVendorHandler(uint32_t id, int subcmd) {
wifi_unregister_vendor_handler(wifiHandle(), id, subcmd);
}
diff --git a/bcmdhd/wifi_hal/wifi_hal.cpp b/bcmdhd/wifi_hal/wifi_hal.cpp
index d3ffe57..c83c482 100755
--- a/bcmdhd/wifi_hal/wifi_hal.cpp
+++ b/bcmdhd/wifi_hal/wifi_hal.cpp
@@ -1043,7 +1043,36 @@
: WifiCommand("SetRSSIMonitorCommand", handle, id), mMax_rssi(max_rssi), mMin_rssi
(min_rssi), mHandler(eh)
{
+ ALOGI("SetRSSIMonitorCommand %p created", this);
}
+
+ virtual ~SetRSSIMonitorCommand() {
+ /*
+ * Mostly, this call will be no effect. However, this could be valid
+ * when object destroy without calling unregisterVendorHandler().
+ * This is added to protect hal crash due to use-after-free.
+ */
+ ALOGI("Try to remove event handler if exist, vendor 0x%0x, subcmd 0x%x",
+ GOOGLE_OUI, GOOGLE_RSSI_MONITOR_EVENT);
+ unregisterVendorHandlerWithoutLock(GOOGLE_OUI, GOOGLE_RSSI_MONITOR_EVENT);
+ ALOGI("SetRSSIMonitorCommand %p destroyed", this);
+ }
+
+ virtual void addRef() {
+ int refs = __sync_add_and_fetch(&mRefs, 1);
+ ALOGI("addRef: WifiCommand %p has %d references", this, refs);
+ }
+
+ virtual void releaseRef() {
+ int refs = __sync_sub_and_fetch(&mRefs, 1);
+ if (refs == 0) {
+ ALOGI("releaseRef: WifiCommand %p has deleted", this);
+ delete this;
+ } else {
+ ALOGI("releaseRef: WifiCommand %p has %d references", this, refs);
+ }
+ }
+
int createRequest(WifiRequest& request, int enable) {
int result = request.create(GOOGLE_OUI, WIFI_SUBCMD_SET_RSSI_MONITOR);
if (result < 0) {
@@ -1759,11 +1788,13 @@
NULL_CHECK_RETURN(cmd, "memory allocation failure", WIFI_ERROR_OUT_OF_MEMORY);
wifi_error result = wifi_register_cmd(handle, id, cmd);
if (result != WIFI_SUCCESS) {
+ ALOGI("wifi_register_cmd() is failed %d", id);
cmd->releaseRef();
return result;
}
result = (wifi_error)cmd->start();
if (result != WIFI_SUCCESS) {
+ ALOGI("start() is failed %d", id);
wifi_unregister_cmd(handle, id);
cmd->releaseRef();
return result;
