From f0fdc816d07b07be00026f3f59a85eaa649d9c52 Mon Sep 17 00:00:00 2001 From: Sungjoon Park Date: Tue, 22 Nov 2022 21:43:34 +0900 Subject: bcmdhd: Fixed Memory overwrite when handling RTT event [Issues] The kernel panic occurs by memory overwrite if the test code is added with incorrect length and id. When checking the argument of ctx, it was defined in various structures. We couldn't know the correct size of ctx in rtt_unpack_xtlv_cbfn function, so require to fix. [Fix] Make a structure(rtt_event_data_info_t) to include bcm_xtlv_t, rtt_result_t and wl_proxd_ftm_session_status_t. Using the structure change to copy memory explicitly. Bug: 257290781, 257289560, 257290396, 254839721, 254840211 Test: Over 3 day RTT aging test has passed. Change-Id: I5140cc4639ac5e9f90aa51818c3b85c9fe874640 Signed-off-by: Sungjoon Park --- dhd_rtt.c | 148 ++++++++++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 105 insertions(+), 43 deletions(-) diff --git a/dhd_rtt.c b/dhd_rtt.c index 0558240..6c6ba2a 100644 --- a/dhd_rtt.c +++ b/dhd_rtt.c @@ -199,6 +199,12 @@ typedef struct ftm_status_map_host_entry { rtt_reason_t rtt_reason; } ftm_status_map_host_entry_t; +typedef struct rtt_event_data_info { + wl_proxd_ftm_session_status_t *session_status; + rtt_result_t *rtt_result; + bcm_xtlv_t *tlv; +} rtt_event_data_info_t; + static uint16 rtt_result_ver(uint16 tlvid, const uint8 *p_data); @@ -844,24 +850,32 @@ rtt_collect_data_event_ver(uint16 len) } } -static void -rtt_collect_event_data_display(uint8 ver, void *ctx, const uint8 *p_data, uint16 len) +static int +rtt_collect_event_data_display(uint8 ver, bcm_xtlv_t *tlv, const uint8 *p_data, uint16 len) { int i; + int ret = BCME_OK; wl_proxd_collect_event_data_v1_t *p_collect_data_v1 = NULL; wl_proxd_collect_event_data_v2_t *p_collect_data_v2 = NULL; wl_proxd_collect_event_data_v3_t *p_collect_data_v3 = NULL; wl_proxd_collect_event_data_v4_t *p_collect_data_v4 = NULL; - if (!ctx || !p_data) { - return; + if (!tlv || !p_data) { + return BCME_ERROR; + } + if (!(len < BCM_XTLV_MAX_DATA_SIZE_EX(BCM_XTLV_OPTION_NONE))) { + return BCME_BUFTOOLONG; } switch (ver) { case WL_PROXD_COLLECT_EVENT_DATA_VERSION_1: DHD_RTT(("\tVERSION_1\n")); - memcpy(ctx, p_data, sizeof(wl_proxd_collect_event_data_v1_t)); - p_collect_data_v1 = (wl_proxd_collect_event_data_v1_t *)ctx; + ret = memcpy_s(tlv->data, tlv->len, p_data, + sizeof(wl_proxd_collect_event_data_v1_t)); + if (ret != BCME_OK) { + break; + } + p_collect_data_v1 = (wl_proxd_collect_event_data_v1_t *)tlv->data; DHD_RTT(("\tH_RX\n")); for (i = 0; i < K_TOF_COLLECT_H_SIZE_20MHZ; i++) { p_collect_data_v1->H_RX[i] = ltoh32_ua(&p_collect_data_v1->H_RX[i]); @@ -882,8 +896,12 @@ rtt_collect_event_data_display(uint8 ver, void *ctx, const uint8 *p_data, uint16 DHD_RTT(("\tphy_err_mask=0x%x\n", p_collect_data_v1->phy_err_mask)); break; case WL_PROXD_COLLECT_EVENT_DATA_VERSION_2: - memcpy(ctx, p_data, sizeof(wl_proxd_collect_event_data_v2_t)); - p_collect_data_v2 = (wl_proxd_collect_event_data_v2_t *)ctx; + ret = memcpy_s(tlv->data, tlv->len, p_data, + sizeof(wl_proxd_collect_event_data_v2_t)); + if (ret != BCME_OK) { + break; + } + p_collect_data_v2 = (wl_proxd_collect_event_data_v2_t *)tlv->data; DHD_RTT(("\tH_RX\n")); for (i = 0; i < K_TOF_COLLECT_H_SIZE_20MHZ; i++) { p_collect_data_v2->H_RX[i] = ltoh32_ua(&p_collect_data_v2->H_RX[i]); @@ -904,8 +922,12 @@ rtt_collect_event_data_display(uint8 ver, void *ctx, const uint8 *p_data, uint16 DHD_RTT(("\tphy_err_mask=0x%x\n", p_collect_data_v2->phy_err_mask)); break; case WL_PROXD_COLLECT_EVENT_DATA_VERSION_3: - memcpy(ctx, p_data, sizeof(wl_proxd_collect_event_data_v3_t)); - p_collect_data_v3 = (wl_proxd_collect_event_data_v3_t *)ctx; + ret = memcpy_s(tlv->data, tlv->len, p_data, + sizeof(wl_proxd_collect_event_data_v3_t)); + if (ret != BCME_OK) { + break; + } + p_collect_data_v3 = (wl_proxd_collect_event_data_v3_t *)tlv->data; switch (p_collect_data_v3->version) { case WL_PROXD_COLLECT_EVENT_DATA_VERSION_3: if (p_collect_data_v3->length != @@ -939,8 +961,12 @@ rtt_collect_event_data_display(uint8 ver, void *ctx, const uint8 *p_data, uint16 } break; case WL_PROXD_COLLECT_EVENT_DATA_VERSION_4: - memcpy(ctx, p_data, sizeof(wl_proxd_collect_event_data_v4_t)); - p_collect_data_v4 = (wl_proxd_collect_event_data_v4_t *)ctx; + ret = memcpy_s(tlv->data, tlv->len, p_data, + sizeof(wl_proxd_collect_event_data_v4_t)); + if (ret != BCME_OK) { + break; + } + p_collect_data_v4 = (wl_proxd_collect_event_data_v4_t *)tlv->data; switch (p_collect_data_v4->version) { case WL_PROXD_COLLECT_EVENT_DATA_VERSION_4: if (p_collect_data_v4->length != @@ -974,6 +1000,7 @@ rtt_collect_event_data_display(uint8 ver, void *ctx, const uint8 *p_data, uint16 } break; } + return ret; } static uint16 @@ -1057,9 +1084,10 @@ rtt_unpack_xtlv_cbfn(void *ctx, const uint8 *p_data, uint16 tlvid, uint16 len) wl_proxd_ftm_session_status_t *p_data_info = NULL; uint32 chan_data_entry = 0; uint16 expected_rtt_result_ver = 0; -#ifdef WL_RTT_LCI - bcm_xtlv_t *tlv = NULL; -#endif /* WL_RTT_LCI */ + + rtt_event_data_info_t *rtt_event_data_info = (rtt_event_data_info_t *)ctx; + rtt_result_t *rtt_result = rtt_event_data_info->rtt_result; + bcm_xtlv_t *tlv = rtt_event_data_info->tlv; BCM_REFERENCE(p_data_info); @@ -1069,17 +1097,21 @@ rtt_unpack_xtlv_cbfn(void *ctx, const uint8 *p_data, uint16 tlvid, uint16 len) case WL_PROXD_TLV_ID_RTT_RESULT_V3: DHD_RTT(("WL_PROXD_TLV_ID_RTT_RESULT\n")); expected_rtt_result_ver = rtt_result_ver(tlvid, p_data); + if (rtt_result == NULL) { + ret = BCME_ERROR; + break; + } switch (expected_rtt_result_ver) { case WL_PROXD_RTT_RESULT_VERSION_1: - ret = dhd_rtt_convert_results_to_host_v1((rtt_result_t *)ctx, + ret = dhd_rtt_convert_results_to_host_v1(rtt_result, p_data, tlvid, len); break; case WL_PROXD_RTT_RESULT_VERSION_2: - ret = dhd_rtt_convert_results_to_host_v2((rtt_result_t *)ctx, + ret = dhd_rtt_convert_results_to_host_v2(rtt_result, p_data, tlvid, len); break; case WL_PROXD_RTT_RESULT_VERSION_3: - ret = dhd_rtt_convert_results_to_host_v3((rtt_result_t *)ctx, + ret = dhd_rtt_convert_results_to_host_v3(rtt_result, p_data, tlvid, len); break; default: @@ -1090,8 +1122,17 @@ rtt_unpack_xtlv_cbfn(void *ctx, const uint8 *p_data, uint16 tlvid, uint16 len) break; case WL_PROXD_TLV_ID_SESSION_STATUS: DHD_RTT(("WL_PROXD_TLV_ID_SESSION_STATUS\n")); - memcpy(ctx, p_data, sizeof(wl_proxd_ftm_session_status_t)); - p_data_info = (wl_proxd_ftm_session_status_t *)ctx; + if (rtt_event_data_info->session_status == NULL) { + ret = BCME_ERROR; + break; + } + ret = memcpy_s(rtt_event_data_info->session_status, + sizeof(wl_proxd_ftm_session_status_t), p_data, len); + if (ret != BCME_OK) { + ret = BCME_BUFTOOSHORT; + break; + } + p_data_info = (wl_proxd_ftm_session_status_t *)rtt_event_data_info->session_status; p_data_info->sid = ltoh16_ua(&p_data_info->sid); p_data_info->state = ltoh16_ua(&p_data_info->state); p_data_info->status = ltoh32_ua(&p_data_info->status); @@ -1109,9 +1150,9 @@ rtt_unpack_xtlv_cbfn(void *ctx, const uint8 *p_data, uint16 tlvid, uint16 len) /* we do not have handle to wl in the context of * xtlv callback without changing the xtlv API. */ - rtt_collect_event_data_display( + ret = rtt_collect_event_data_display( rtt_collect_data_event_ver(len), - ctx, p_data, len); + tlv, p_data, len); break; case WL_PROXD_TLV_ID_COLLECT_CHAN_DATA: GCC_DIAGNOSTIC_PUSH_SUPPRESS_CAST(); @@ -1131,25 +1172,35 @@ rtt_unpack_xtlv_cbfn(void *ctx, const uint8 *p_data, uint16 tlvid, uint16 len) break; #ifdef WL_RTT_LCI case WL_PROXD_TLV_ID_LCI: - tlv = (bcm_xtlv_t *)ctx; DHD_RTT(("WL_PROXD_TLV_ID_LCI, IE data=%lx, len=%d\n", (unsigned long)p_data, len)); rtt_prhex("", p_data, len); if (tlv) { tlv->id = WL_PROXD_TLV_ID_LCI; + ret = memcpy_s(tlv->data, tlv->len, p_data, len); tlv->len = len; - (void)memcpy_s(tlv->data, len, p_data, len); + if (ret != BCME_OK) { + break; + } + } + else { + ret = BCME_ERROR; } break; case WL_PROXD_TLV_ID_CIVIC: - tlv = (bcm_xtlv_t *)ctx; DHD_RTT(("WL_PROXD_TLV_ID_CIVIC, IE data=%lx, len=%d\n", (unsigned long)p_data, len)); rtt_prhex("", p_data, len); if (tlv) { tlv->id = WL_PROXD_TLV_ID_CIVIC; tlv->len = len; - (void)memcpy_s(tlv->data, len, p_data, len); + ret = memcpy_s(tlv->data, tlv->len, p_data, len); + if (ret != BCME_OK) { + break; + } + } + else { + ret = BCME_ERROR; } break; #endif /* WL_RTT_LCI */ @@ -4386,9 +4437,12 @@ dhd_rtt_parse_result_event(wl_proxd_event_t *proxd_ev_data, int tlvs_len, rtt_result_t *rtt_result) { int ret = BCME_OK; + rtt_event_data_info_t rtt_event_data_info; + memset(&rtt_event_data_info, 0, sizeof(rtt_event_data_info_t)); + rtt_event_data_info.rtt_result = rtt_result; /* unpack TLVs and invokes the cbfn to print the event content TLVs */ - ret = bcm_unpack_xtlv_buf((void *) rtt_result, + ret = bcm_unpack_xtlv_buf((void *) &rtt_event_data_info, (uint8 *)&proxd_ev_data->tlvs[0], tlvs_len, BCM_XTLV_OPTION_ALIGN32, rtt_unpack_xtlv_cbfn); if (ret != BCME_OK) { @@ -4639,7 +4693,6 @@ dhd_rtt_event_handler(dhd_pub_t *dhd, wl_event_msg_t *event, void *event_data) uint16 version; wl_proxd_event_t *p_event; wl_proxd_event_type_t event_type; - wl_proxd_ftm_session_status_t session_status; const ftm_strmap_entry_t *p_loginfo; rtt_result_t *rtt_result; #ifdef WL_CFG80211 @@ -4648,6 +4701,7 @@ dhd_rtt_event_handler(dhd_pub_t *dhd, wl_event_msg_t *event, void *event_data) bool is_new = TRUE; rtt_target_info_t *target = NULL; #endif /* WL_CFG80211 */ + rtt_event_data_info_t rtt_event_data_info; DHD_RTT(("Enter %s \n", __FUNCTION__)); NULL_CHECK(dhd, "dhd is NULL", ret); @@ -4785,9 +4839,17 @@ dhd_rtt_event_handler(dhd_pub_t *dhd, wl_event_msg_t *event, void *event_data) #endif /* WL_CFG80211 */ if (tlvs_len > 0) { /* unpack TLVs and invokes the cbfn to print the event content TLVs */ - ret = bcm_unpack_xtlv_buf((void *) &session_status, + rtt_event_data_info.session_status = (wl_proxd_ftm_session_status_t *) + MALLOCZ(dhd->osh, sizeof(wl_proxd_ftm_session_status_t)); + if (!rtt_event_data_info.session_status) { + ret = -ENOMEM; + goto exit; + } + ret = bcm_unpack_xtlv_buf((void *) &rtt_event_data_info, (uint8 *)&p_event->tlvs[0], tlvs_len, BCM_XTLV_OPTION_ALIGN32, rtt_unpack_xtlv_cbfn); + MFREE(dhd->osh, rtt_event_data_info.session_status, + sizeof(wl_proxd_ftm_session_status_t)); if (ret != BCME_OK) { DHD_RTT_ERR(("%s : Failed to unpack xtlv for an event\n", __FUNCTION__)); @@ -4827,19 +4889,19 @@ dhd_rtt_event_handler(dhd_pub_t *dhd, wl_event_msg_t *event, void *event_data) case WL_PROXD_EVENT_CIVIC_MEAS_REP: DHD_RTT(("WL_PROXD_EVENT_LCI/CIVIC_MEAS_REP\n")); if (tlvs_len > 0) { - void *buffer = NULL; - if (!(buffer = (void *)MALLOCZ(dhd->osh, tlvs_len))) { + if (!(rtt_event_data_info.tlv = (void *)MALLOCZ(dhd->osh, tlvs_len))) { ret = -ENOMEM; goto exit; } + rtt_event_data_info.tlv->len = tlvs_len - BCM_XTLV_HDR_SIZE; /* unpack TLVs and invokes the cbfn to print the event content TLVs */ - ret = bcm_unpack_xtlv_buf(buffer, + ret = bcm_unpack_xtlv_buf(&rtt_event_data_info, (uint8 *)&p_event->tlvs[0], tlvs_len, BCM_XTLV_OPTION_NONE, rtt_unpack_xtlv_cbfn); if (ret != BCME_OK) { DHD_RTT_ERR(("%s : Failed to unpack xtlv for event %d\n", __FUNCTION__, event_type)); - MFREE(dhd->osh, buffer, tlvs_len); + MFREE(dhd->osh, rtt_event_data_info.tlv, tlvs_len); goto exit; } if (event_type == WL_PROXD_EVENT_LCI_MEAS_REP) { @@ -4849,7 +4911,7 @@ dhd_rtt_event_handler(dhd_pub_t *dhd, wl_event_msg_t *event, void *event_data) target->LCI->len + BCM_XTLV_HDR_SIZE); } DHD_RTT(("WL_PROXD_EVENT_LCI_MEAS_REP: cache the LCI tlv\n")); - target->LCI = (bcm_xtlv_t *)buffer; + target->LCI = rtt_event_data_info.tlv; } else { /* free previous one and update it */ if (target->LCR) { @@ -4857,7 +4919,7 @@ dhd_rtt_event_handler(dhd_pub_t *dhd, wl_event_msg_t *event, void *event_data) target->LCR->len + BCM_XTLV_HDR_SIZE); } DHD_RTT(("WL_PROXD_EVENT_CIVIC_MEAS_REP: cache the LCR tlv\n")); - target->LCR = (bcm_xtlv_t *)buffer; + target->LCR = rtt_event_data_info.tlv; } } break; @@ -4868,16 +4930,16 @@ dhd_rtt_event_handler(dhd_pub_t *dhd, wl_event_msg_t *event, void *event_data) case WL_PROXD_EVENT_COLLECT: DHD_RTT(("WL_PROXD_EVENT_COLLECT\n")); if (tlvs_len > 0) { - void *buffer = NULL; - if (!(buffer = (void *)MALLOCZ(dhd->osh, tlvs_len))) { + if (!(rtt_event_data_info.tlv = (void *)MALLOCZ(dhd->osh, tlvs_len))) { ret = -ENOMEM; goto exit; } + rtt_event_data_info.tlv->len = tlvs_len - BCM_XTLV_HDR_SIZE; /* unpack TLVs and invokes the cbfn to print the event content TLVs */ - ret = bcm_unpack_xtlv_buf(buffer, + ret = bcm_unpack_xtlv_buf(&rtt_event_data_info, (uint8 *)&p_event->tlvs[0], tlvs_len, BCM_XTLV_OPTION_NONE, rtt_unpack_xtlv_cbfn); - MFREE(dhd->osh, buffer, tlvs_len); + MFREE(dhd->osh, rtt_event_data_info.tlv, tlvs_len); if (ret != BCME_OK) { DHD_RTT_ERR(("%s : Failed to unpack xtlv for event %d\n", __FUNCTION__, event_type)); @@ -4888,16 +4950,16 @@ dhd_rtt_event_handler(dhd_pub_t *dhd, wl_event_msg_t *event, void *event_data) case WL_PROXD_EVENT_MF_STATS: DHD_RTT(("WL_PROXD_EVENT_MF_STATS\n")); if (tlvs_len > 0) { - void *buffer = NULL; - if (!(buffer = (void *)MALLOCZ(dhd->osh, tlvs_len))) { + if (!(rtt_event_data_info.tlv = (void *)MALLOCZ(dhd->osh, tlvs_len))) { ret = -ENOMEM; goto exit; } + rtt_event_data_info.tlv->len = tlvs_len - BCM_XTLV_HDR_SIZE; /* unpack TLVs and invokes the cbfn to print the event content TLVs */ - ret = bcm_unpack_xtlv_buf(buffer, + ret = bcm_unpack_xtlv_buf(&rtt_event_data_info, (uint8 *)&p_event->tlvs[0], tlvs_len, BCM_XTLV_OPTION_NONE, rtt_unpack_xtlv_cbfn); - MFREE(dhd->osh, buffer, tlvs_len); + MFREE(dhd->osh, rtt_event_data_info.tlv, tlvs_len); if (ret != BCME_OK) { DHD_RTT_ERR(("%s : Failed to unpack xtlv for event %d\n", __FUNCTION__, event_type)); -- cgit v1.2.3 From 309e2f4e451c27d429281ea4e8d71e8daafa18bc Mon Sep 17 00:00:00 2001 From: Sungjoon Park Date: Mon, 12 Dec 2022 16:00:50 +0900 Subject: bcmdhd: Fixed Memory OOB Read/Write in function wl_update_hidden_ap_ie In wl_update_hidden_ap_ie of wl_cfgscan.c, there is a possible out of bounds write due to a missing bounds check. Fix: 1. Added bounds check. 2. If the ie_offset + ie_length is not matched with length then bi might be corrupted. Ignoring that AP bi update. Bug: 254029309 Test: BRCM Internal test is finished without regression. Change-Id: Ibd638e27a09f657f903c46e8c4712732af47cfc9 Signed-off-by: Sungjoon Park --- wl_cfgscan.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 46 insertions(+), 12 deletions(-) diff --git a/wl_cfgscan.c b/wl_cfgscan.c index f5fd713..d665851 100644 --- a/wl_cfgscan.c +++ b/wl_cfgscan.c @@ -216,6 +216,7 @@ static void wl_update_hidden_ap_ie(wl_bss_info_v109_t *bi, const u8 *ie_stream, int32 ssid_len = MIN(bi->SSID_len, DOT11_MAX_SSID_LEN); int32 remaining_ie_buf_len, available_buffer_len, unused_buf_len; /* cfg80211_find_ie defined in kernel returning const u8 */ + int ret = 0; GCC_DIAGNOSTIC_PUSH_SUPPRESS_CAST(); ssidie = (u8 *)cfg80211_find_ie(WLAN_EID_SSID, ie_stream, *ie_size); @@ -259,10 +260,19 @@ static void wl_update_hidden_ap_ie(wl_bss_info_v109_t *bi, const u8 *ie_stream, */ if ((update_ssid && (ssid_len > ssidie[1])) && (unused_buf_len > ssid_len)) { WL_INFORM_MEM(("Changing the SSID Info.\n")); - memmove(ssidie + ssid_len + 2, - (ssidie + 2) + ssidie[1], - remaining_ie_buf_len); - memcpy(ssidie + 2, bi->SSID, ssid_len); + ret = memmove_s(ssidie + ssid_len + 2, available_buffer_len, + (ssidie + 2) + ssidie[1], remaining_ie_buf_len); + if (ret) { + WL_ERR(("SSID Info memmove failed:%d, destsz:%d, n:%d\n", + ret, available_buffer_len, remaining_ie_buf_len)); + return; + } + ret = memcpy_s(ssidie + 2, DOT11_MAX_SSID_LEN, bi->SSID, ssid_len); + if (ret) { + WL_ERR(("SSID Info memcpy failed:%d, destsz:%d, n:%d\n", + ret, DOT11_MAX_SSID_LEN, ssid_len)); + return; + } *ie_size = *ie_size + ssid_len - ssidie[1]; ssidie[1] = ssid_len; } else if (ssid_len < ssidie[1]) { @@ -271,8 +281,14 @@ static void wl_update_hidden_ap_ie(wl_bss_info_v109_t *bi, const u8 *ie_stream, } return; } - if (*(ssidie + 2) == '\0') - memcpy(ssidie + 2, bi->SSID, ssid_len); + if (*(ssidie + 2) == '\0') { + ret = memcpy_s(ssidie + 2, DOT11_MAX_SSID_LEN, bi->SSID, ssid_len); + if (ret) { + WL_ERR(("memcopy failed:%d, destsz:%d, n:%d\n", + ret, DOT11_MAX_SSID_LEN, ssid_len)); + return; + } + } return; } @@ -280,12 +296,19 @@ static s32 wl_mrg_ie(struct bcm_cfg80211 *cfg, u8 *ie_stream, u16 ie_size) { struct wl_ie *ie = wl_to_ie(cfg); s32 err = 0; + int ret = 0; if (unlikely(ie->offset + ie_size > WL_TLV_INFO_MAX)) { WL_ERR(("ei_stream crosses buffer boundary\n")); return -ENOSPC; } - memcpy(&ie->buf[ie->offset], ie_stream, ie_size); + ret = memcpy_s(&ie->buf[ie->offset], (sizeof(ie->buf) - ie->offset), + ie_stream, ie_size); + if (ret) { + WL_ERR(("memcpy failed:%d, destsz: %lu, n: %d\n", + ret, (sizeof(ie->buf) - ie->offset), ie_size)); + return BCME_ERROR; + } ie->offset += ie_size; return err; @@ -339,6 +362,12 @@ s32 wl_inform_single_bss(struct bcm_cfg80211 *cfg, wl_bss_info_v109_t *bi, bool return err; } + if (bi->length < (bi->ie_offset + bi->ie_length)) { + WL_ERR(("IE length is not Valid. IE offse:%d, len:%d\n", + bi->ie_offset, bi->ie_length)); + return -EINVAL; + } + if (bi->SSID_len > IEEE80211_MAX_SSID_LEN) { WL_ERR(("wrong SSID len:%d\n", bi->SSID_len)); return -EINVAL; @@ -655,6 +684,7 @@ wl_bcnrecv_result_handler(struct bcm_cfg80211 *cfg, bcm_struct_cfgdev *cfgdev, struct wiphy *wiphy = NULL; wl_bcnrecv_result_t *bcn_recv = NULL; struct timespec64 ts; + int ret = 0; if (!bi) { WL_ERR(("%s: bi is NULL\n", __func__)); err = BCME_NORESOURCE; @@ -677,11 +707,15 @@ wl_bcnrecv_result_handler(struct bcm_cfg80211 *cfg, bcm_struct_cfgdev *cfgdev, WL_ERR(("Failed to allocate memory\n")); return -ENOMEM; } - /* Returning void here as copy size does not exceed dest size of SSID */ - (void)memcpy_s((char *)bcn_recv->SSID, DOT11_MAX_SSID_LEN, - (char *)bi->SSID, DOT11_MAX_SSID_LEN); - /* Returning void here as copy size does not exceed dest size of ETH_LEN */ - (void)memcpy_s(&bcn_recv->BSSID, ETHER_ADDR_LEN, &bi->BSSID, ETH_ALEN); + ret = memcpy_s((char *)bcn_recv->SSID, sizeof(bcn_recv->SSID), + (char *)bi->SSID, bi->SSID_len); + if (ret) { + WL_ERR(("memcpy failed:%d, destsz:%lu, n:%d\n", + ret, sizeof(bcn_recv->SSID), bi->SSID_len)); + err = BCME_ERROR; + goto exit; + } + eacopy(&bi->BSSID, &bcn_recv->BSSID); bcn_recv->channel = wf_chspec_ctlchan( wl_chspec_driver_to_host(bi->chanspec)); bcn_recv->beacon_interval = bi->beacon_period; -- cgit v1.2.3 From 758722db62609e423528708492fcea4ee82fbc77 Mon Sep 17 00:00:00 2001 From: Sungjoon Park Date: Mon, 12 Dec 2022 16:03:02 +0900 Subject: bcmdhd: Fixed Memory Overwrite in function dhd_prot_ioctcmplt_process In dhd_prot_ioctcmplt_process of dhd_msgbuf.c, there is a possible out of bounds write due to improper input validation Fix: 1. Added bounds check 2. Limited the copy length to dest length. Bug: 254028518 Test: BRCM Internal test is finished without regression. Change-Id: I7d000282c6732ff0963751284ac6331c7cc48d8b Signed-off-by: Sungjoon Park --- dhd_msgbuf.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/dhd_msgbuf.c b/dhd_msgbuf.c index 0900fb7..4bc1d1a 100644 --- a/dhd_msgbuf.c +++ b/dhd_msgbuf.c @@ -7750,6 +7750,7 @@ dhd_prot_ioctcmplt_process(dhd_pub_t *dhd, void *msg) #ifdef REPORT_FATAL_TIMEOUTS uint16 dhd_xt_id; #endif + int ret = 0; /* Check for ioctl timeout induce flag, which is set by firing * dhd iovar to induce IOCTL timeout. If flag is set, @@ -7845,11 +7846,18 @@ dhd_prot_ioctcmplt_process(dhd_pub_t *dhd, void *msg) pkt_id, xt_id, prot->ioctl_status, prot->ioctl_resplen)); if (prot->ioctl_resplen > 0) { + uint16 copy_len = MIN(prot->ioctl_resplen, prot->retbuf.len); #ifndef IOCTLRESP_USE_CONSTMEM - bcopy(PKTDATA(dhd->osh, pkt), prot->retbuf.va, prot->ioctl_resplen); + ret = memcpy_s(prot->retbuf.va, prot->retbuf.len, PKTDATA(dhd->osh, pkt), copy_len); #else - bcopy(pkt, prot->retbuf.va, prot->ioctl_resplen); + ret = memcpy_s(prot->retbuf.va, prot->retbuf.len, pkt, copy_len); #endif /* !IOCTLRESP_USE_CONSTMEM */ + if (ret) { + DHD_ERROR(("memcpy failed:%d, destsz:%d, n:%u\n", + ret, prot->retbuf.len, copy_len)); + dhd_wakeup_ioctl_event(dhd, IOCTL_RETURN_ON_ERROR); + goto exit; + } } /* wake up any dhd_os_ioctl_resp_wait() */ -- cgit v1.2.3 From 836ae82407c9a72519c90ddad9998ed82a38ae53 Mon Sep 17 00:00:00 2001 From: Sungjoon Park Date: Mon, 12 Dec 2022 16:04:57 +0900 Subject: bcmdhd: Fixed Memory Overwrite in function add_roam_cache_list In add_roam_cache_list of wl_roam.c, there is a possible out of bounds write due to a missing bounds check. Fix: 1. Added bounds check 2. If SSID_len is bigger than 32, do not update that list in the roam cache list. Bug: 254028776 Test: BRCM Internal test is finished without regression. Change-Id: Ifaf4a5c963e89dde3fed39888c4fa83d093f5e25 Signed-off-by: Sungjoon Park --- wl_roam.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/wl_roam.c b/wl_roam.c index a53697d..05043d7 100644 --- a/wl_roam.c +++ b/wl_roam.c @@ -178,7 +178,7 @@ void reset_roam_cache(struct bcm_cfg80211 *cfg) static void add_roam_cache_list(uint8 *SSID, uint32 SSID_len, chanspec_t chanspec) { - int i; + int i, ret = 0; uint8 channel; char chanbuf[CHANSPEC_STR_LEN]; @@ -186,6 +186,11 @@ add_roam_cache_list(uint8 *SSID, uint32 SSID_len, chanspec_t chanspec) return; } + if (SSID_len > DOT11_MAX_SSID_LEN) { + WL_ERR(("SSID len %u out of bounds [0-32]\n", SSID_len)); + return; + } + for (i = 0; i < n_roam_cache; i++) { if ((roam_cache[i].ssid_len == SSID_len) && (roam_cache[i].chanspec == chanspec) && @@ -197,10 +202,16 @@ add_roam_cache_list(uint8 *SSID, uint32 SSID_len, chanspec_t chanspec) roam_cache[n_roam_cache].ssid_len = SSID_len; channel = wf_chspec_ctlchan(chanspec); - WL_DBG(("CHSPEC = %s, CTL %d SSID %s\n", + WL_DBG(("CHSPEC = %s, CTL %d SSID %.32s\n", wf_chspec_ntoa_ex(chanspec, chanbuf), channel, SSID)); roam_cache[n_roam_cache].chanspec = CHSPEC_BAND(chanspec) | band_bw | channel; - (void)memcpy_s(roam_cache[n_roam_cache].ssid, SSID_len, SSID, SSID_len); + ret = memcpy_s(roam_cache[n_roam_cache].ssid, + sizeof(roam_cache[n_roam_cache].ssid), SSID, SSID_len); + if (ret) { + WL_ERR(("memcpy failed:%d, destsz:%lu, n:%d\n", + ret, sizeof(roam_cache[n_roam_cache].ssid), SSID_len)); + return; + } n_roam_cache++; } -- cgit v1.2.3 From bed359aa046e46bf7af1d9d4a18f536447349a37 Mon Sep 17 00:00:00 2001 From: Sungjoon Park Date: Tue, 22 Nov 2022 21:43:34 +0900 Subject: bcmdhd: Fixed Memory overwrite when handling RTT event [Issues] The kernel panic occurs by memory overwrite if the test code is added with incorrect length and id. When checking the argument of ctx, it was defined in various structures. We couldn't know the correct size of ctx in rtt_unpack_xtlv_cbfn function, so require to fix. [Fix] Make a structure(rtt_event_data_info_t) to include bcm_xtlv_t, rtt_result_t and wl_proxd_ftm_session_status_t. Using the structure change to copy memory explicitly. Bug: 257290781, 257289560, 257290396, 254839721, 254840211 Test: Over 3 day RTT aging test has passed. Change-Id: I5140cc4639ac5e9f90aa51818c3b85c9fe874640 Signed-off-by: Sungjoon Park --- dhd_rtt.c | 148 ++++++++++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 105 insertions(+), 43 deletions(-) diff --git a/dhd_rtt.c b/dhd_rtt.c index 0558240..6c6ba2a 100644 --- a/dhd_rtt.c +++ b/dhd_rtt.c @@ -199,6 +199,12 @@ typedef struct ftm_status_map_host_entry { rtt_reason_t rtt_reason; } ftm_status_map_host_entry_t; +typedef struct rtt_event_data_info { + wl_proxd_ftm_session_status_t *session_status; + rtt_result_t *rtt_result; + bcm_xtlv_t *tlv; +} rtt_event_data_info_t; + static uint16 rtt_result_ver(uint16 tlvid, const uint8 *p_data); @@ -844,24 +850,32 @@ rtt_collect_data_event_ver(uint16 len) } } -static void -rtt_collect_event_data_display(uint8 ver, void *ctx, const uint8 *p_data, uint16 len) +static int +rtt_collect_event_data_display(uint8 ver, bcm_xtlv_t *tlv, const uint8 *p_data, uint16 len) { int i; + int ret = BCME_OK; wl_proxd_collect_event_data_v1_t *p_collect_data_v1 = NULL; wl_proxd_collect_event_data_v2_t *p_collect_data_v2 = NULL; wl_proxd_collect_event_data_v3_t *p_collect_data_v3 = NULL; wl_proxd_collect_event_data_v4_t *p_collect_data_v4 = NULL; - if (!ctx || !p_data) { - return; + if (!tlv || !p_data) { + return BCME_ERROR; + } + if (!(len < BCM_XTLV_MAX_DATA_SIZE_EX(BCM_XTLV_OPTION_NONE))) { + return BCME_BUFTOOLONG; } switch (ver) { case WL_PROXD_COLLECT_EVENT_DATA_VERSION_1: DHD_RTT(("\tVERSION_1\n")); - memcpy(ctx, p_data, sizeof(wl_proxd_collect_event_data_v1_t)); - p_collect_data_v1 = (wl_proxd_collect_event_data_v1_t *)ctx; + ret = memcpy_s(tlv->data, tlv->len, p_data, + sizeof(wl_proxd_collect_event_data_v1_t)); + if (ret != BCME_OK) { + break; + } + p_collect_data_v1 = (wl_proxd_collect_event_data_v1_t *)tlv->data; DHD_RTT(("\tH_RX\n")); for (i = 0; i < K_TOF_COLLECT_H_SIZE_20MHZ; i++) { p_collect_data_v1->H_RX[i] = ltoh32_ua(&p_collect_data_v1->H_RX[i]); @@ -882,8 +896,12 @@ rtt_collect_event_data_display(uint8 ver, void *ctx, const uint8 *p_data, uint16 DHD_RTT(("\tphy_err_mask=0x%x\n", p_collect_data_v1->phy_err_mask)); break; case WL_PROXD_COLLECT_EVENT_DATA_VERSION_2: - memcpy(ctx, p_data, sizeof(wl_proxd_collect_event_data_v2_t)); - p_collect_data_v2 = (wl_proxd_collect_event_data_v2_t *)ctx; + ret = memcpy_s(tlv->data, tlv->len, p_data, + sizeof(wl_proxd_collect_event_data_v2_t)); + if (ret != BCME_OK) { + break; + } + p_collect_data_v2 = (wl_proxd_collect_event_data_v2_t *)tlv->data; DHD_RTT(("\tH_RX\n")); for (i = 0; i < K_TOF_COLLECT_H_SIZE_20MHZ; i++) { p_collect_data_v2->H_RX[i] = ltoh32_ua(&p_collect_data_v2->H_RX[i]); @@ -904,8 +922,12 @@ rtt_collect_event_data_display(uint8 ver, void *ctx, const uint8 *p_data, uint16 DHD_RTT(("\tphy_err_mask=0x%x\n", p_collect_data_v2->phy_err_mask)); break; case WL_PROXD_COLLECT_EVENT_DATA_VERSION_3: - memcpy(ctx, p_data, sizeof(wl_proxd_collect_event_data_v3_t)); - p_collect_data_v3 = (wl_proxd_collect_event_data_v3_t *)ctx; + ret = memcpy_s(tlv->data, tlv->len, p_data, + sizeof(wl_proxd_collect_event_data_v3_t)); + if (ret != BCME_OK) { + break; + } + p_collect_data_v3 = (wl_proxd_collect_event_data_v3_t *)tlv->data; switch (p_collect_data_v3->version) { case WL_PROXD_COLLECT_EVENT_DATA_VERSION_3: if (p_collect_data_v3->length != @@ -939,8 +961,12 @@ rtt_collect_event_data_display(uint8 ver, void *ctx, const uint8 *p_data, uint16 } break; case WL_PROXD_COLLECT_EVENT_DATA_VERSION_4: - memcpy(ctx, p_data, sizeof(wl_proxd_collect_event_data_v4_t)); - p_collect_data_v4 = (wl_proxd_collect_event_data_v4_t *)ctx; + ret = memcpy_s(tlv->data, tlv->len, p_data, + sizeof(wl_proxd_collect_event_data_v4_t)); + if (ret != BCME_OK) { + break; + } + p_collect_data_v4 = (wl_proxd_collect_event_data_v4_t *)tlv->data; switch (p_collect_data_v4->version) { case WL_PROXD_COLLECT_EVENT_DATA_VERSION_4: if (p_collect_data_v4->length != @@ -974,6 +1000,7 @@ rtt_collect_event_data_display(uint8 ver, void *ctx, const uint8 *p_data, uint16 } break; } + return ret; } static uint16 @@ -1057,9 +1084,10 @@ rtt_unpack_xtlv_cbfn(void *ctx, const uint8 *p_data, uint16 tlvid, uint16 len) wl_proxd_ftm_session_status_t *p_data_info = NULL; uint32 chan_data_entry = 0; uint16 expected_rtt_result_ver = 0; -#ifdef WL_RTT_LCI - bcm_xtlv_t *tlv = NULL; -#endif /* WL_RTT_LCI */ + + rtt_event_data_info_t *rtt_event_data_info = (rtt_event_data_info_t *)ctx; + rtt_result_t *rtt_result = rtt_event_data_info->rtt_result; + bcm_xtlv_t *tlv = rtt_event_data_info->tlv; BCM_REFERENCE(p_data_info); @@ -1069,17 +1097,21 @@ rtt_unpack_xtlv_cbfn(void *ctx, const uint8 *p_data, uint16 tlvid, uint16 len) case WL_PROXD_TLV_ID_RTT_RESULT_V3: DHD_RTT(("WL_PROXD_TLV_ID_RTT_RESULT\n")); expected_rtt_result_ver = rtt_result_ver(tlvid, p_data); + if (rtt_result == NULL) { + ret = BCME_ERROR; + break; + } switch (expected_rtt_result_ver) { case WL_PROXD_RTT_RESULT_VERSION_1: - ret = dhd_rtt_convert_results_to_host_v1((rtt_result_t *)ctx, + ret = dhd_rtt_convert_results_to_host_v1(rtt_result, p_data, tlvid, len); break; case WL_PROXD_RTT_RESULT_VERSION_2: - ret = dhd_rtt_convert_results_to_host_v2((rtt_result_t *)ctx, + ret = dhd_rtt_convert_results_to_host_v2(rtt_result, p_data, tlvid, len); break; case WL_PROXD_RTT_RESULT_VERSION_3: - ret = dhd_rtt_convert_results_to_host_v3((rtt_result_t *)ctx, + ret = dhd_rtt_convert_results_to_host_v3(rtt_result, p_data, tlvid, len); break; default: @@ -1090,8 +1122,17 @@ rtt_unpack_xtlv_cbfn(void *ctx, const uint8 *p_data, uint16 tlvid, uint16 len) break; case WL_PROXD_TLV_ID_SESSION_STATUS: DHD_RTT(("WL_PROXD_TLV_ID_SESSION_STATUS\n")); - memcpy(ctx, p_data, sizeof(wl_proxd_ftm_session_status_t)); - p_data_info = (wl_proxd_ftm_session_status_t *)ctx; + if (rtt_event_data_info->session_status == NULL) { + ret = BCME_ERROR; + break; + } + ret = memcpy_s(rtt_event_data_info->session_status, + sizeof(wl_proxd_ftm_session_status_t), p_data, len); + if (ret != BCME_OK) { + ret = BCME_BUFTOOSHORT; + break; + } + p_data_info = (wl_proxd_ftm_session_status_t *)rtt_event_data_info->session_status; p_data_info->sid = ltoh16_ua(&p_data_info->sid); p_data_info->state = ltoh16_ua(&p_data_info->state); p_data_info->status = ltoh32_ua(&p_data_info->status); @@ -1109,9 +1150,9 @@ rtt_unpack_xtlv_cbfn(void *ctx, const uint8 *p_data, uint16 tlvid, uint16 len) /* we do not have handle to wl in the context of * xtlv callback without changing the xtlv API. */ - rtt_collect_event_data_display( + ret = rtt_collect_event_data_display( rtt_collect_data_event_ver(len), - ctx, p_data, len); + tlv, p_data, len); break; case WL_PROXD_TLV_ID_COLLECT_CHAN_DATA: GCC_DIAGNOSTIC_PUSH_SUPPRESS_CAST(); @@ -1131,25 +1172,35 @@ rtt_unpack_xtlv_cbfn(void *ctx, const uint8 *p_data, uint16 tlvid, uint16 len) break; #ifdef WL_RTT_LCI case WL_PROXD_TLV_ID_LCI: - tlv = (bcm_xtlv_t *)ctx; DHD_RTT(("WL_PROXD_TLV_ID_LCI, IE data=%lx, len=%d\n", (unsigned long)p_data, len)); rtt_prhex("", p_data, len); if (tlv) { tlv->id = WL_PROXD_TLV_ID_LCI; + ret = memcpy_s(tlv->data, tlv->len, p_data, len); tlv->len = len; - (void)memcpy_s(tlv->data, len, p_data, len); + if (ret != BCME_OK) { + break; + } + } + else { + ret = BCME_ERROR; } break; case WL_PROXD_TLV_ID_CIVIC: - tlv = (bcm_xtlv_t *)ctx; DHD_RTT(("WL_PROXD_TLV_ID_CIVIC, IE data=%lx, len=%d\n", (unsigned long)p_data, len)); rtt_prhex("", p_data, len); if (tlv) { tlv->id = WL_PROXD_TLV_ID_CIVIC; tlv->len = len; - (void)memcpy_s(tlv->data, len, p_data, len); + ret = memcpy_s(tlv->data, tlv->len, p_data, len); + if (ret != BCME_OK) { + break; + } + } + else { + ret = BCME_ERROR; } break; #endif /* WL_RTT_LCI */ @@ -4386,9 +4437,12 @@ dhd_rtt_parse_result_event(wl_proxd_event_t *proxd_ev_data, int tlvs_len, rtt_result_t *rtt_result) { int ret = BCME_OK; + rtt_event_data_info_t rtt_event_data_info; + memset(&rtt_event_data_info, 0, sizeof(rtt_event_data_info_t)); + rtt_event_data_info.rtt_result = rtt_result; /* unpack TLVs and invokes the cbfn to print the event content TLVs */ - ret = bcm_unpack_xtlv_buf((void *) rtt_result, + ret = bcm_unpack_xtlv_buf((void *) &rtt_event_data_info, (uint8 *)&proxd_ev_data->tlvs[0], tlvs_len, BCM_XTLV_OPTION_ALIGN32, rtt_unpack_xtlv_cbfn); if (ret != BCME_OK) { @@ -4639,7 +4693,6 @@ dhd_rtt_event_handler(dhd_pub_t *dhd, wl_event_msg_t *event, void *event_data) uint16 version; wl_proxd_event_t *p_event; wl_proxd_event_type_t event_type; - wl_proxd_ftm_session_status_t session_status; const ftm_strmap_entry_t *p_loginfo; rtt_result_t *rtt_result; #ifdef WL_CFG80211 @@ -4648,6 +4701,7 @@ dhd_rtt_event_handler(dhd_pub_t *dhd, wl_event_msg_t *event, void *event_data) bool is_new = TRUE; rtt_target_info_t *target = NULL; #endif /* WL_CFG80211 */ + rtt_event_data_info_t rtt_event_data_info; DHD_RTT(("Enter %s \n", __FUNCTION__)); NULL_CHECK(dhd, "dhd is NULL", ret); @@ -4785,9 +4839,17 @@ dhd_rtt_event_handler(dhd_pub_t *dhd, wl_event_msg_t *event, void *event_data) #endif /* WL_CFG80211 */ if (tlvs_len > 0) { /* unpack TLVs and invokes the cbfn to print the event content TLVs */ - ret = bcm_unpack_xtlv_buf((void *) &session_status, + rtt_event_data_info.session_status = (wl_proxd_ftm_session_status_t *) + MALLOCZ(dhd->osh, sizeof(wl_proxd_ftm_session_status_t)); + if (!rtt_event_data_info.session_status) { + ret = -ENOMEM; + goto exit; + } + ret = bcm_unpack_xtlv_buf((void *) &rtt_event_data_info, (uint8 *)&p_event->tlvs[0], tlvs_len, BCM_XTLV_OPTION_ALIGN32, rtt_unpack_xtlv_cbfn); + MFREE(dhd->osh, rtt_event_data_info.session_status, + sizeof(wl_proxd_ftm_session_status_t)); if (ret != BCME_OK) { DHD_RTT_ERR(("%s : Failed to unpack xtlv for an event\n", __FUNCTION__)); @@ -4827,19 +4889,19 @@ dhd_rtt_event_handler(dhd_pub_t *dhd, wl_event_msg_t *event, void *event_data) case WL_PROXD_EVENT_CIVIC_MEAS_REP: DHD_RTT(("WL_PROXD_EVENT_LCI/CIVIC_MEAS_REP\n")); if (tlvs_len > 0) { - void *buffer = NULL; - if (!(buffer = (void *)MALLOCZ(dhd->osh, tlvs_len))) { + if (!(rtt_event_data_info.tlv = (void *)MALLOCZ(dhd->osh, tlvs_len))) { ret = -ENOMEM; goto exit; } + rtt_event_data_info.tlv->len = tlvs_len - BCM_XTLV_HDR_SIZE; /* unpack TLVs and invokes the cbfn to print the event content TLVs */ - ret = bcm_unpack_xtlv_buf(buffer, + ret = bcm_unpack_xtlv_buf(&rtt_event_data_info, (uint8 *)&p_event->tlvs[0], tlvs_len, BCM_XTLV_OPTION_NONE, rtt_unpack_xtlv_cbfn); if (ret != BCME_OK) { DHD_RTT_ERR(("%s : Failed to unpack xtlv for event %d\n", __FUNCTION__, event_type)); - MFREE(dhd->osh, buffer, tlvs_len); + MFREE(dhd->osh, rtt_event_data_info.tlv, tlvs_len); goto exit; } if (event_type == WL_PROXD_EVENT_LCI_MEAS_REP) { @@ -4849,7 +4911,7 @@ dhd_rtt_event_handler(dhd_pub_t *dhd, wl_event_msg_t *event, void *event_data) target->LCI->len + BCM_XTLV_HDR_SIZE); } DHD_RTT(("WL_PROXD_EVENT_LCI_MEAS_REP: cache the LCI tlv\n")); - target->LCI = (bcm_xtlv_t *)buffer; + target->LCI = rtt_event_data_info.tlv; } else { /* free previous one and update it */ if (target->LCR) { @@ -4857,7 +4919,7 @@ dhd_rtt_event_handler(dhd_pub_t *dhd, wl_event_msg_t *event, void *event_data) target->LCR->len + BCM_XTLV_HDR_SIZE); } DHD_RTT(("WL_PROXD_EVENT_CIVIC_MEAS_REP: cache the LCR tlv\n")); - target->LCR = (bcm_xtlv_t *)buffer; + target->LCR = rtt_event_data_info.tlv; } } break; @@ -4868,16 +4930,16 @@ dhd_rtt_event_handler(dhd_pub_t *dhd, wl_event_msg_t *event, void *event_data) case WL_PROXD_EVENT_COLLECT: DHD_RTT(("WL_PROXD_EVENT_COLLECT\n")); if (tlvs_len > 0) { - void *buffer = NULL; - if (!(buffer = (void *)MALLOCZ(dhd->osh, tlvs_len))) { + if (!(rtt_event_data_info.tlv = (void *)MALLOCZ(dhd->osh, tlvs_len))) { ret = -ENOMEM; goto exit; } + rtt_event_data_info.tlv->len = tlvs_len - BCM_XTLV_HDR_SIZE; /* unpack TLVs and invokes the cbfn to print the event content TLVs */ - ret = bcm_unpack_xtlv_buf(buffer, + ret = bcm_unpack_xtlv_buf(&rtt_event_data_info, (uint8 *)&p_event->tlvs[0], tlvs_len, BCM_XTLV_OPTION_NONE, rtt_unpack_xtlv_cbfn); - MFREE(dhd->osh, buffer, tlvs_len); + MFREE(dhd->osh, rtt_event_data_info.tlv, tlvs_len); if (ret != BCME_OK) { DHD_RTT_ERR(("%s : Failed to unpack xtlv for event %d\n", __FUNCTION__, event_type)); @@ -4888,16 +4950,16 @@ dhd_rtt_event_handler(dhd_pub_t *dhd, wl_event_msg_t *event, void *event_data) case WL_PROXD_EVENT_MF_STATS: DHD_RTT(("WL_PROXD_EVENT_MF_STATS\n")); if (tlvs_len > 0) { - void *buffer = NULL; - if (!(buffer = (void *)MALLOCZ(dhd->osh, tlvs_len))) { + if (!(rtt_event_data_info.tlv = (void *)MALLOCZ(dhd->osh, tlvs_len))) { ret = -ENOMEM; goto exit; } + rtt_event_data_info.tlv->len = tlvs_len - BCM_XTLV_HDR_SIZE; /* unpack TLVs and invokes the cbfn to print the event content TLVs */ - ret = bcm_unpack_xtlv_buf(buffer, + ret = bcm_unpack_xtlv_buf(&rtt_event_data_info, (uint8 *)&p_event->tlvs[0], tlvs_len, BCM_XTLV_OPTION_NONE, rtt_unpack_xtlv_cbfn); - MFREE(dhd->osh, buffer, tlvs_len); + MFREE(dhd->osh, rtt_event_data_info.tlv, tlvs_len); if (ret != BCME_OK) { DHD_RTT_ERR(("%s : Failed to unpack xtlv for event %d\n", __FUNCTION__, event_type)); -- cgit v1.2.3 From 31b7bb4d289272bdcde29b8e37cc2a5c2d5e2760 Mon Sep 17 00:00:00 2001 From: Sungjoon Park Date: Mon, 12 Dec 2022 16:00:50 +0900 Subject: bcmdhd: Fixed Memory OOB Read/Write in function wl_update_hidden_ap_ie In wl_update_hidden_ap_ie of wl_cfgscan.c, there is a possible out of bounds write due to a missing bounds check. Fix: 1. Added bounds check. 2. If the ie_offset + ie_length is not matched with length then bi might be corrupted. Ignoring that AP bi update. Bug: 254029309 Test: BRCM Internal test is finished without regression. Change-Id: Ibd638e27a09f657f903c46e8c4712732af47cfc9 Signed-off-by: Sungjoon Park --- wl_cfgscan.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 46 insertions(+), 12 deletions(-) diff --git a/wl_cfgscan.c b/wl_cfgscan.c index f5fd713..d665851 100644 --- a/wl_cfgscan.c +++ b/wl_cfgscan.c @@ -216,6 +216,7 @@ static void wl_update_hidden_ap_ie(wl_bss_info_v109_t *bi, const u8 *ie_stream, int32 ssid_len = MIN(bi->SSID_len, DOT11_MAX_SSID_LEN); int32 remaining_ie_buf_len, available_buffer_len, unused_buf_len; /* cfg80211_find_ie defined in kernel returning const u8 */ + int ret = 0; GCC_DIAGNOSTIC_PUSH_SUPPRESS_CAST(); ssidie = (u8 *)cfg80211_find_ie(WLAN_EID_SSID, ie_stream, *ie_size); @@ -259,10 +260,19 @@ static void wl_update_hidden_ap_ie(wl_bss_info_v109_t *bi, const u8 *ie_stream, */ if ((update_ssid && (ssid_len > ssidie[1])) && (unused_buf_len > ssid_len)) { WL_INFORM_MEM(("Changing the SSID Info.\n")); - memmove(ssidie + ssid_len + 2, - (ssidie + 2) + ssidie[1], - remaining_ie_buf_len); - memcpy(ssidie + 2, bi->SSID, ssid_len); + ret = memmove_s(ssidie + ssid_len + 2, available_buffer_len, + (ssidie + 2) + ssidie[1], remaining_ie_buf_len); + if (ret) { + WL_ERR(("SSID Info memmove failed:%d, destsz:%d, n:%d\n", + ret, available_buffer_len, remaining_ie_buf_len)); + return; + } + ret = memcpy_s(ssidie + 2, DOT11_MAX_SSID_LEN, bi->SSID, ssid_len); + if (ret) { + WL_ERR(("SSID Info memcpy failed:%d, destsz:%d, n:%d\n", + ret, DOT11_MAX_SSID_LEN, ssid_len)); + return; + } *ie_size = *ie_size + ssid_len - ssidie[1]; ssidie[1] = ssid_len; } else if (ssid_len < ssidie[1]) { @@ -271,8 +281,14 @@ static void wl_update_hidden_ap_ie(wl_bss_info_v109_t *bi, const u8 *ie_stream, } return; } - if (*(ssidie + 2) == '\0') - memcpy(ssidie + 2, bi->SSID, ssid_len); + if (*(ssidie + 2) == '\0') { + ret = memcpy_s(ssidie + 2, DOT11_MAX_SSID_LEN, bi->SSID, ssid_len); + if (ret) { + WL_ERR(("memcopy failed:%d, destsz:%d, n:%d\n", + ret, DOT11_MAX_SSID_LEN, ssid_len)); + return; + } + } return; } @@ -280,12 +296,19 @@ static s32 wl_mrg_ie(struct bcm_cfg80211 *cfg, u8 *ie_stream, u16 ie_size) { struct wl_ie *ie = wl_to_ie(cfg); s32 err = 0; + int ret = 0; if (unlikely(ie->offset + ie_size > WL_TLV_INFO_MAX)) { WL_ERR(("ei_stream crosses buffer boundary\n")); return -ENOSPC; } - memcpy(&ie->buf[ie->offset], ie_stream, ie_size); + ret = memcpy_s(&ie->buf[ie->offset], (sizeof(ie->buf) - ie->offset), + ie_stream, ie_size); + if (ret) { + WL_ERR(("memcpy failed:%d, destsz: %lu, n: %d\n", + ret, (sizeof(ie->buf) - ie->offset), ie_size)); + return BCME_ERROR; + } ie->offset += ie_size; return err; @@ -339,6 +362,12 @@ s32 wl_inform_single_bss(struct bcm_cfg80211 *cfg, wl_bss_info_v109_t *bi, bool return err; } + if (bi->length < (bi->ie_offset + bi->ie_length)) { + WL_ERR(("IE length is not Valid. IE offse:%d, len:%d\n", + bi->ie_offset, bi->ie_length)); + return -EINVAL; + } + if (bi->SSID_len > IEEE80211_MAX_SSID_LEN) { WL_ERR(("wrong SSID len:%d\n", bi->SSID_len)); return -EINVAL; @@ -655,6 +684,7 @@ wl_bcnrecv_result_handler(struct bcm_cfg80211 *cfg, bcm_struct_cfgdev *cfgdev, struct wiphy *wiphy = NULL; wl_bcnrecv_result_t *bcn_recv = NULL; struct timespec64 ts; + int ret = 0; if (!bi) { WL_ERR(("%s: bi is NULL\n", __func__)); err = BCME_NORESOURCE; @@ -677,11 +707,15 @@ wl_bcnrecv_result_handler(struct bcm_cfg80211 *cfg, bcm_struct_cfgdev *cfgdev, WL_ERR(("Failed to allocate memory\n")); return -ENOMEM; } - /* Returning void here as copy size does not exceed dest size of SSID */ - (void)memcpy_s((char *)bcn_recv->SSID, DOT11_MAX_SSID_LEN, - (char *)bi->SSID, DOT11_MAX_SSID_LEN); - /* Returning void here as copy size does not exceed dest size of ETH_LEN */ - (void)memcpy_s(&bcn_recv->BSSID, ETHER_ADDR_LEN, &bi->BSSID, ETH_ALEN); + ret = memcpy_s((char *)bcn_recv->SSID, sizeof(bcn_recv->SSID), + (char *)bi->SSID, bi->SSID_len); + if (ret) { + WL_ERR(("memcpy failed:%d, destsz:%lu, n:%d\n", + ret, sizeof(bcn_recv->SSID), bi->SSID_len)); + err = BCME_ERROR; + goto exit; + } + eacopy(&bi->BSSID, &bcn_recv->BSSID); bcn_recv->channel = wf_chspec_ctlchan( wl_chspec_driver_to_host(bi->chanspec)); bcn_recv->beacon_interval = bi->beacon_period; -- cgit v1.2.3 From 56beb29852101f342c12f355bf89639a107336cc Mon Sep 17 00:00:00 2001 From: Sungjoon Park Date: Mon, 12 Dec 2022 16:03:02 +0900 Subject: bcmdhd: Fixed Memory Overwrite in function dhd_prot_ioctcmplt_process In dhd_prot_ioctcmplt_process of dhd_msgbuf.c, there is a possible out of bounds write due to improper input validation Fix: 1. Added bounds check 2. Limited the copy length to dest length. Bug: 254028518 Test: BRCM Internal test is finished without regression. Change-Id: I7d000282c6732ff0963751284ac6331c7cc48d8b Signed-off-by: Sungjoon Park --- dhd_msgbuf.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/dhd_msgbuf.c b/dhd_msgbuf.c index 0900fb7..4bc1d1a 100644 --- a/dhd_msgbuf.c +++ b/dhd_msgbuf.c @@ -7750,6 +7750,7 @@ dhd_prot_ioctcmplt_process(dhd_pub_t *dhd, void *msg) #ifdef REPORT_FATAL_TIMEOUTS uint16 dhd_xt_id; #endif + int ret = 0; /* Check for ioctl timeout induce flag, which is set by firing * dhd iovar to induce IOCTL timeout. If flag is set, @@ -7845,11 +7846,18 @@ dhd_prot_ioctcmplt_process(dhd_pub_t *dhd, void *msg) pkt_id, xt_id, prot->ioctl_status, prot->ioctl_resplen)); if (prot->ioctl_resplen > 0) { + uint16 copy_len = MIN(prot->ioctl_resplen, prot->retbuf.len); #ifndef IOCTLRESP_USE_CONSTMEM - bcopy(PKTDATA(dhd->osh, pkt), prot->retbuf.va, prot->ioctl_resplen); + ret = memcpy_s(prot->retbuf.va, prot->retbuf.len, PKTDATA(dhd->osh, pkt), copy_len); #else - bcopy(pkt, prot->retbuf.va, prot->ioctl_resplen); + ret = memcpy_s(prot->retbuf.va, prot->retbuf.len, pkt, copy_len); #endif /* !IOCTLRESP_USE_CONSTMEM */ + if (ret) { + DHD_ERROR(("memcpy failed:%d, destsz:%d, n:%u\n", + ret, prot->retbuf.len, copy_len)); + dhd_wakeup_ioctl_event(dhd, IOCTL_RETURN_ON_ERROR); + goto exit; + } } /* wake up any dhd_os_ioctl_resp_wait() */ -- cgit v1.2.3 From aa4626311e2926e28ab4c7ce0599be7c5df769ab Mon Sep 17 00:00:00 2001 From: Sungjoon Park Date: Mon, 12 Dec 2022 16:04:57 +0900 Subject: bcmdhd: Fixed Memory Overwrite in function add_roam_cache_list In add_roam_cache_list of wl_roam.c, there is a possible out of bounds write due to a missing bounds check. Fix: 1. Added bounds check 2. If SSID_len is bigger than 32, do not update that list in the roam cache list. Bug: 254028776 Test: BRCM Internal test is finished without regression. Change-Id: Ifaf4a5c963e89dde3fed39888c4fa83d093f5e25 Signed-off-by: Sungjoon Park --- wl_roam.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/wl_roam.c b/wl_roam.c index a53697d..05043d7 100644 --- a/wl_roam.c +++ b/wl_roam.c @@ -178,7 +178,7 @@ void reset_roam_cache(struct bcm_cfg80211 *cfg) static void add_roam_cache_list(uint8 *SSID, uint32 SSID_len, chanspec_t chanspec) { - int i; + int i, ret = 0; uint8 channel; char chanbuf[CHANSPEC_STR_LEN]; @@ -186,6 +186,11 @@ add_roam_cache_list(uint8 *SSID, uint32 SSID_len, chanspec_t chanspec) return; } + if (SSID_len > DOT11_MAX_SSID_LEN) { + WL_ERR(("SSID len %u out of bounds [0-32]\n", SSID_len)); + return; + } + for (i = 0; i < n_roam_cache; i++) { if ((roam_cache[i].ssid_len == SSID_len) && (roam_cache[i].chanspec == chanspec) && @@ -197,10 +202,16 @@ add_roam_cache_list(uint8 *SSID, uint32 SSID_len, chanspec_t chanspec) roam_cache[n_roam_cache].ssid_len = SSID_len; channel = wf_chspec_ctlchan(chanspec); - WL_DBG(("CHSPEC = %s, CTL %d SSID %s\n", + WL_DBG(("CHSPEC = %s, CTL %d SSID %.32s\n", wf_chspec_ntoa_ex(chanspec, chanbuf), channel, SSID)); roam_cache[n_roam_cache].chanspec = CHSPEC_BAND(chanspec) | band_bw | channel; - (void)memcpy_s(roam_cache[n_roam_cache].ssid, SSID_len, SSID, SSID_len); + ret = memcpy_s(roam_cache[n_roam_cache].ssid, + sizeof(roam_cache[n_roam_cache].ssid), SSID, SSID_len); + if (ret) { + WL_ERR(("memcpy failed:%d, destsz:%lu, n:%d\n", + ret, sizeof(roam_cache[n_roam_cache].ssid), SSID_len)); + return; + } n_roam_cache++; } -- cgit v1.2.3