17.1: switch to flamefire's ASB topics

This gets us ~9 extra patches

Signed-off-by: Tad <tad@spotco.us>
This commit is contained in:
Tad 2023-06-17 14:22:37 -04:00
parent a07133a064
commit 41e2669884
No known key found for this signature in database
GPG key ID: B286E9F57A07424B
74 changed files with 23 additions and 8185 deletions

View file

@ -1,41 +0,0 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Hui Peng <phui@google.com>
Date: Wed, 28 Dec 2022 00:32:37 +0000
Subject: [PATCH] Fix an OOB Write bug in gatt_check_write_long_terminate
this is the backport of Ifffa2c7f679c4ef72dbdb6b1f3378ca506680084
Bug: 258652631
Test: manual
Tag: #security
Ignore-AOSP-First: security
Change-Id: Ic84122f07cbc198c676d366e39606621b7cb4e66
(cherry picked from commit 9b17660bfd6f0f41cb9400ce0236d76c83605e03)
Merged-In: Ic84122f07cbc198c676d366e39606621b7cb4e66
---
stack/gatt/gatt_cl.cc | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/stack/gatt/gatt_cl.cc b/stack/gatt/gatt_cl.cc
index 3115317da..db41c5f9f 100644
--- a/stack/gatt/gatt_cl.cc
+++ b/stack/gatt/gatt_cl.cc
@@ -572,7 +572,8 @@ void gatt_process_prep_write_rsp(tGATT_TCB& tcb, tGATT_CLCB* p_clcb,
LOG(ERROR) << StringPrintf("value resp op_code = %s len = %d",
gatt_dbg_op_name(op_code), len);
- if (len < GATT_PREP_WRITE_RSP_MIN_LEN) {
+ if (len < GATT_PREP_WRITE_RSP_MIN_LEN ||
+ len > GATT_PREP_WRITE_RSP_MIN_LEN + sizeof(value.value)) {
LOG(ERROR) << "illegal prepare write response length, discard";
gatt_end_operation(p_clcb, GATT_INVALID_PDU, &value);
return;
@@ -581,7 +582,7 @@ void gatt_process_prep_write_rsp(tGATT_TCB& tcb, tGATT_CLCB* p_clcb,
STREAM_TO_UINT16(value.handle, p);
STREAM_TO_UINT16(value.offset, p);
- value.len = len - 4;
+ value.len = len - GATT_PREP_WRITE_RSP_MIN_LEN;
memcpy(value.value, p, value.len);

View file

@ -1,39 +0,0 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Hui Peng <phui@google.com>
Date: Mon, 2 Jan 2023 22:05:45 +0000
Subject: [PATCH] Fix an OOB access bug in A2DP_BuildMediaPayloadHeaderSbc
In A2DP_BuildCodecHeaderSbc when p_buf->offset is 0, the
`-=` operation on it may result in integer underflow and
OOB write with the computed pointer passed to
A2DP_BuildMediaPayloadHeaderSbc.
This is a backport of I45320085b1e458d3b0e0d86162a35aaaae7b34cb
Test: atest net_test_stack_a2dp_codecs_native
Ignore-AOSP-First: security
Tag:#security
Bug: 186803518
Change-Id: I4ff1a1de71884b8de23008b2569fdea3650e85ec
(cherry picked from commit a710300216be4a86373a65c6a685aeef8509cfa7)
Merged-In: I4ff1a1de71884b8de23008b2569fdea3650e85ec
---
stack/a2dp/a2dp_sbc.cc | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/stack/a2dp/a2dp_sbc.cc b/stack/a2dp/a2dp_sbc.cc
index 4c48993c4..5036eec2e 100644
--- a/stack/a2dp/a2dp_sbc.cc
+++ b/stack/a2dp/a2dp_sbc.cc
@@ -704,6 +704,11 @@ bool A2DP_BuildCodecHeaderSbc(UNUSED_ATTR const uint8_t* p_codec_info,
BT_HDR* p_buf, uint16_t frames_per_packet) {
uint8_t* p;
+ // there is a timestamp right following p_buf
+ if (p_buf->offset < 4 + A2DP_SBC_MPL_HDR_LEN) {
+ return false;
+ }
+
p_buf->offset -= A2DP_SBC_MPL_HDR_LEN;
p = (uint8_t*)(p_buf + 1) + p_buf->offset;
p_buf->len += A2DP_SBC_MPL_HDR_LEN;

View file

@ -1,75 +0,0 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Hui Peng <phui@google.com>
Date: Wed, 4 Jan 2023 22:45:13 +0000
Subject: [PATCH] Fix an OOB write in SDP_AddAttribute
When the `attr_pad` becomes full, it is possible
that un index of `-1` is computed write
a zero byte to `p_val`, rusulting OOB write.
```
p_val[SDP_MAX_PAD_LEN - p_rec->free_pad_ptr - 1] = '\0';
```
This is a backport of I937d22a2df26fca1d7f06b10182c4e713ddfed1b
Bug: 261867748
Test: manual
Tag: #security
Ignore-AOSP-First: security
Change-Id: Ibdda754e628cfc9d1706c14db114919a15d8d6b1
(cherry picked from commit cc527a97f78a2999a0156a579e488afe9e3675b2)
Merged-In: Ibdda754e628cfc9d1706c14db114919a15d8d6b1
---
stack/sdp/sdp_db.cc | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/stack/sdp/sdp_db.cc b/stack/sdp/sdp_db.cc
index ea5b84d23..4130ae71a 100644
--- a/stack/sdp/sdp_db.cc
+++ b/stack/sdp/sdp_db.cc
@@ -362,6 +362,11 @@ bool SDP_AddAttribute(uint32_t handle, uint16_t attr_id, uint8_t attr_type,
uint16_t xx, yy, zz;
tSDP_RECORD* p_rec = &sdp_cb.server_db.record[0];
+ if (p_val == nullptr) {
+ SDP_TRACE_WARNING("Trying to add attribute with p_val == nullptr, skipped");
+ return (false);
+ }
+
if (sdp_cb.trace_level >= BT_TRACE_LEVEL_DEBUG) {
if ((attr_type == UINT_DESC_TYPE) ||
(attr_type == TWO_COMP_INT_DESC_TYPE) ||
@@ -398,6 +403,13 @@ bool SDP_AddAttribute(uint32_t handle, uint16_t attr_id, uint8_t attr_type,
if (p_rec->record_handle == handle) {
tSDP_ATTRIBUTE* p_attr = &p_rec->attribute[0];
+ // error out early, no need to look up
+ if (p_rec->free_pad_ptr >= SDP_MAX_PAD_LEN) {
+ SDP_TRACE_ERROR("the free pad for SDP record with handle %d is "
+ "full, skip adding the attribute", handle);
+ return (false);
+ }
+
/* Found the record. Now, see if the attribute already exists */
for (xx = 0; xx < p_rec->num_attributes; xx++, p_attr++) {
/* The attribute exists. replace it */
@@ -437,15 +449,13 @@ bool SDP_AddAttribute(uint32_t handle, uint16_t attr_id, uint8_t attr_type,
attr_len = 0;
}
- if ((attr_len > 0) && (p_val != 0)) {
+ if (attr_len > 0) {
p_attr->len = attr_len;
memcpy(&p_rec->attr_pad[p_rec->free_pad_ptr], p_val, (size_t)attr_len);
p_attr->value_ptr = &p_rec->attr_pad[p_rec->free_pad_ptr];
p_rec->free_pad_ptr += attr_len;
- } else if ((attr_len == 0 &&
- p_attr->len !=
- 0) || /* if truncate to 0 length, simply don't add */
- p_val == 0) {
+ } else if (attr_len == 0 && p_attr->len != 0) {
+ /* if truncate to 0 length, simply don't add */
SDP_TRACE_ERROR(
"SDP_AddAttribute fail, length exceed maximum: ID %d: attr_len:%d ",
attr_id, attr_len);

View file

@ -1,54 +0,0 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Hui Peng <phui@google.com>
Date: Thu, 8 Dec 2022 01:08:11 +0000
Subject: [PATCH] Fix OOB access in avdt_scb_hdl_pkt_no_frag
This is a back port of the following 2 CLs:
- Id13b1ebde8f603123c8b7a49922b2f1378ab788f
- If0c7b25f2e6cb4531bbb6254e176e8ad1b5c5fb4
Regression test: I9c87e30ed58e7ad6a34ab7c96b0a8fb06324ad54
Bug: 142546355 258057241
Test: atest net_test_stack_avdtp
Ignore-AOSP-First: security
Change-Id: Ie1707385d6452ece47915c153f4faaa1c8a287c9
(cherry picked from commit b0b968e8c6214e20a5dc3617d66567225df0884f)
Merged-In: Ie1707385d6452ece47915c153f4faaa1c8a287c9
---
stack/avdt/avdt_scb_act.cc | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/stack/avdt/avdt_scb_act.cc b/stack/avdt/avdt_scb_act.cc
index ce53c45eb..f2de4ba35 100644
--- a/stack/avdt/avdt_scb_act.cc
+++ b/stack/avdt/avdt_scb_act.cc
@@ -255,19 +255,24 @@ void avdt_scb_hdl_pkt_no_frag(AvdtpScb* p_scb, tAVDT_SCB_EVT* p_data) {
if (offset > len) goto length_error;
p += 2;
BE_STREAM_TO_UINT16(ex_len, p);
- offset += ex_len * 4;
p += ex_len * 4;
}
+ if ((p - p_start) >= len) {
+ AVDT_TRACE_WARNING("%s: handling malformatted packet: ex_len too large", __func__);
+ osi_free_and_reset((void**)&p_data->p_pkt);
+ return;
+ }
+ offset = p - p_start;
+
/* adjust length for any padding at end of packet */
if (o_p) {
/* padding length in last byte of packet */
- pad_len = *(p_start + p_data->p_pkt->len);
+ pad_len = *(p_start + len - 1);
}
/* do sanity check */
- if ((offset > p_data->p_pkt->len) ||
- ((pad_len + offset) > p_data->p_pkt->len)) {
+ if (pad_len >= (len - offset)) {
AVDT_TRACE_WARNING("Got bad media packet");
osi_free_and_reset((void**)&p_data->p_pkt);
}

View file

@ -1,34 +0,0 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Hui Peng <phui@google.com>
Date: Fri, 20 Jan 2023 19:39:30 +0000
Subject: [PATCH] Fix an OOB bug in register_notification_rsp
This is a backport of I901d973a736678d7f3cc816ddf0cbbcbbd1fe93f
to rvc-dev.
Bug: 245916076
Test: manual
Ignore-AOSP-First: security
Change-Id: I37a9f45e707702b2ec52b5a2d572f177f2911765
(cherry picked from commit 901e34203c6280d414cbfa3978de04fd6515ffdf)
Merged-In: I37a9f45e707702b2ec52b5a2d572f177f2911765
---
btif/src/btif_rc.cc | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/btif/src/btif_rc.cc b/btif/src/btif_rc.cc
index 575d83e37..be29e559b 100644
--- a/btif/src/btif_rc.cc
+++ b/btif/src/btif_rc.cc
@@ -1892,6 +1892,11 @@ static bt_status_t register_notification_rsp(
dump_rc_notification_event_id(event_id));
std::unique_lock<std::mutex> lock(btif_rc_cb.lock);
+ if (event_id > MAX_RC_NOTIFICATIONS) {
+ BTIF_TRACE_ERROR("Invalid event id");
+ return BT_STATUS_PARM_INVALID;
+ }
+
memset(&(avrc_rsp.reg_notif), 0, sizeof(tAVRC_REG_NOTIF_RSP));
avrc_rsp.reg_notif.event_id = event_id;

View file

@ -1,107 +0,0 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Brian Delwiche <delwiche@google.com>
Date: Tue, 11 Oct 2022 21:23:22 +0000
Subject: [PATCH] Prevent use-after-free of HID reports
BTA sends the the HID report pointer to BTIF and deallocates it immediately.
This is now prevented by providing a deep copy callback function for HID
reports when tranferring context from BTA to BTIF.
This is a backport of change Icef7a7ed1185b4283ee4fe4f812ca154d8f1b825,
already merged on T for b/227620181.
Bug: 228837201
Test: Validated against researcher POC, ran BT unit tests, played audio
manually.
Tag: #security
Ignore-AOSP-First: Security
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:874c495c886cd8722625756dc5fd0634b16b4f42)
Merged-In: Ib837f395883de2369207f1b3b974d6bff02dcb19
Change-Id: Ib837f395883de2369207f1b3b974d6bff02dcb19
---
btif/src/btif_hh.cc | 50 ++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 45 insertions(+), 5 deletions(-)
diff --git a/btif/src/btif_hh.cc b/btif/src/btif_hh.cc
index 650923a3c..5c57ee80c 100644
--- a/btif/src/btif_hh.cc
+++ b/btif/src/btif_hh.cc
@@ -1096,6 +1096,38 @@ static void btif_hh_upstreams_evt(uint16_t event, char* p_param) {
}
}
+/*******************************************************************************
+ *
+ * Function btif_hh_hsdata_rpt_copy_cb
+ *
+ * Description Deep copies the tBTA_HH_HSDATA structure
+ *
+ * Returns void
+ *
+ ******************************************************************************/
+
+static void btif_hh_hsdata_rpt_copy_cb(uint16_t event, char* p_dest,
+ char* p_src) {
+ tBTA_HH_HSDATA* p_dst_data = (tBTA_HH_HSDATA*)p_dest;
+ tBTA_HH_HSDATA* p_src_data = (tBTA_HH_HSDATA*)p_src;
+ BT_HDR* hdr;
+
+ if (!p_src) {
+ BTIF_TRACE_ERROR("%s: Nothing to copy", __func__);
+ return;
+ }
+
+ memcpy(p_dst_data, p_src_data, sizeof(tBTA_HH_HSDATA));
+
+ hdr = p_src_data->rsp_data.p_rpt_data;
+ if (hdr != NULL) {
+ uint8_t* p_data = ((uint8_t*)p_dst_data) + sizeof(tBTA_HH_HSDATA);
+ memcpy(p_data, hdr, BT_HDR_SIZE + hdr->offset + hdr->len);
+
+ p_dst_data->rsp_data.p_rpt_data = (BT_HDR*)p_data;
+ }
+}
+
/*******************************************************************************
*
* Function bte_hh_evt
@@ -1109,6 +1141,7 @@ static void btif_hh_upstreams_evt(uint16_t event, char* p_param) {
void bte_hh_evt(tBTA_HH_EVT event, tBTA_HH* p_data) {
bt_status_t status;
int param_len = 0;
+ tBTIF_COPY_CBACK* p_copy_cback = NULL;
if (BTA_HH_ENABLE_EVT == event)
param_len = sizeof(tBTA_HH_STATUS);
@@ -1120,11 +1153,18 @@ void bte_hh_evt(tBTA_HH_EVT event, tBTA_HH* p_data) {
param_len = sizeof(tBTA_HH_CBDATA);
else if (BTA_HH_GET_DSCP_EVT == event)
param_len = sizeof(tBTA_HH_DEV_DSCP_INFO);
- else if ((BTA_HH_GET_PROTO_EVT == event) || (BTA_HH_GET_RPT_EVT == event) ||
- (BTA_HH_GET_IDLE_EVT == event))
+ else if ((BTA_HH_GET_PROTO_EVT == event) || (BTA_HH_GET_IDLE_EVT == event))
+ param_len = sizeof(tBTA_HH_HSDATA);
+ else if (BTA_HH_GET_RPT_EVT == event) {
+ BT_HDR* hdr = p_data->hs_data.rsp_data.p_rpt_data;
param_len = sizeof(tBTA_HH_HSDATA);
- else if ((BTA_HH_SET_PROTO_EVT == event) || (BTA_HH_SET_RPT_EVT == event) ||
- (BTA_HH_VC_UNPLUG_EVT == event) || (BTA_HH_SET_IDLE_EVT == event))
+
+ if (hdr != NULL) {
+ p_copy_cback = btif_hh_hsdata_rpt_copy_cb;
+ param_len += BT_HDR_SIZE + hdr->offset + hdr->len;
+ }
+ } else if ((BTA_HH_SET_PROTO_EVT == event) || (BTA_HH_SET_RPT_EVT == event) ||
+ (BTA_HH_VC_UNPLUG_EVT == event) || (BTA_HH_SET_IDLE_EVT == event))
param_len = sizeof(tBTA_HH_CBDATA);
else if ((BTA_HH_ADD_DEV_EVT == event) || (BTA_HH_RMV_DEV_EVT == event))
param_len = sizeof(tBTA_HH_DEV_INFO);
@@ -1133,7 +1173,7 @@ void bte_hh_evt(tBTA_HH_EVT event, tBTA_HH* p_data) {
/* switch context to btif task context (copy full union size for convenience)
*/
status = btif_transfer_context(btif_hh_upstreams_evt, (uint16_t)event,
- (char*)p_data, param_len, NULL);
+ (char*)p_data, param_len, p_copy_cback);
/* catch any failed context transfers */
ASSERTC(status == BT_STATUS_SUCCESS, "context transfer failed", status);

View file

@ -1,141 +0,0 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Brian Delwiche <delwiche@google.com>
Date: Tue, 21 Mar 2023 22:35:35 +0000
Subject: [PATCH] Revert "Revert "[RESTRICT AUTOMERGE] Validate buffer length
in sdpu_build_uuid_seq""
This reverts commit 487a1079078f3717fdc4665c19a45eca5b3ec5e6.
Reason for revert: Reinstate original change for QPR
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:a681067af2ea4565543238db3025d749923f63ec)
Merged-In: If0528519a29dc73ff99163098da2a05592ab15d8
Change-Id: If0528519a29dc73ff99163098da2a05592ab15d8
---
stack/sdp/sdp_discovery.cc | 65 ++++++++++++++++++++++++++++++++++----
1 file changed, 58 insertions(+), 7 deletions(-)
diff --git a/stack/sdp/sdp_discovery.cc b/stack/sdp/sdp_discovery.cc
index f5938e423..553e068f5 100644
--- a/stack/sdp/sdp_discovery.cc
+++ b/stack/sdp/sdp_discovery.cc
@@ -70,10 +70,15 @@ static uint8_t* add_attr(uint8_t* p, uint8_t* p_end, tSDP_DISCOVERY_DB* p_db,
*
******************************************************************************/
static uint8_t* sdpu_build_uuid_seq(uint8_t* p_out, uint16_t num_uuids,
- Uuid* p_uuid_list) {
+ Uuid* p_uuid_list, uint16_t& bytes_left) {
uint16_t xx;
uint8_t* p_len;
+ if (bytes_left < 2) {
+ DCHECK(0) << "SDP: No space for data element header";
+ return (p_out);
+ }
+
/* First thing is the data element header */
UINT8_TO_BE_STREAM(p_out, (DATA_ELE_SEQ_DESC_TYPE << 3) | SIZE_IN_NEXT_BYTE);
@@ -81,9 +86,20 @@ static uint8_t* sdpu_build_uuid_seq(uint8_t* p_out, uint16_t num_uuids,
p_len = p_out;
p_out += 1;
+ /* Account for data element header and length */
+ bytes_left -= 2;
+
/* Now, loop through and put in all the UUID(s) */
for (xx = 0; xx < num_uuids; xx++, p_uuid_list++) {
int len = p_uuid_list->GetShortestRepresentationSize();
+
+ if (len + 1 > bytes_left) {
+ DCHECK(0) << "SDP: Too many UUIDs for internal buffer";
+ break;
+ } else {
+ bytes_left -= (len + 1);
+ }
+
if (len == Uuid::kNumBytes16) {
UINT8_TO_BE_STREAM(p_out, (UUID_DESC_TYPE << 3) | SIZE_TWO_BYTES);
UINT16_TO_BE_STREAM(p_out, p_uuid_list->As16Bit());
@@ -120,6 +136,7 @@ static void sdp_snd_service_search_req(tCONN_CB* p_ccb, uint8_t cont_len,
uint8_t *p, *p_start, *p_param_len;
BT_HDR* p_cmd = (BT_HDR*)osi_malloc(SDP_DATA_BUF_SIZE);
uint16_t param_len;
+ uint16_t bytes_left = SDP_DATA_BUF_SIZE;
/* Prepare the buffer for sending the packet to L2CAP */
p_cmd->offset = L2CAP_MIN_OFFSET;
@@ -134,13 +151,30 @@ static void sdp_snd_service_search_req(tCONN_CB* p_ccb, uint8_t cont_len,
p_param_len = p;
p += 2;
-/* Build the UID sequence. */
+ /* Account for header size, max service record count and
+ * continuation state */
+ const uint16_t base_bytes = (sizeof(BT_HDR) + L2CAP_MIN_OFFSET +
+ 3u + /* service search request header */
+ 2u + /* param len */
+ 3u + ((p_cont) ? cont_len : 0));
+
+ if (base_bytes > bytes_left) {
+ DCHECK(0) << "SDP: Overran SDP data buffer";
+ osi_free(p_cmd);
+ return;
+ }
+
+ bytes_left -= base_bytes;
+
+ /* Build the UID sequence. */
#if (SDP_BROWSE_PLUS == TRUE)
p = sdpu_build_uuid_seq(p, 1,
- &p_ccb->p_db->uuid_filters[p_ccb->cur_uuid_idx]);
+ &p_ccb->p_db->uuid_filters[p_ccb->cur_uuid_idx],
+ bytes_left);
#else
+ /* Build the UID sequence. */
p = sdpu_build_uuid_seq(p, p_ccb->p_db->num_uuid_filters,
- p_ccb->p_db->uuid_filters);
+ p_ccb->p_db->uuid_filters, bytes_left);
#endif
/* Set max service record count */
@@ -575,6 +609,7 @@ static void process_service_search_attr_rsp(tCONN_CB* p_ccb, uint8_t* p_reply,
if ((cont_request_needed) || (!p_reply)) {
BT_HDR* p_msg = (BT_HDR*)osi_malloc(SDP_DATA_BUF_SIZE);
uint8_t* p;
+ uint16_t bytes_left = SDP_DATA_BUF_SIZE;
p_msg->offset = L2CAP_MIN_OFFSET;
p = p_start = (uint8_t*)(p_msg + 1) + L2CAP_MIN_OFFSET;
@@ -588,13 +623,29 @@ static void process_service_search_attr_rsp(tCONN_CB* p_ccb, uint8_t* p_reply,
p_param_len = p;
p += 2;
-/* Build the UID sequence. */
+ /* Account for header size, max service record count and
+ * continuation state */
+ const uint16_t base_bytes = (sizeof(BT_HDR) + L2CAP_MIN_OFFSET +
+ 3u + /* service search request header */
+ 2u + /* param len */
+ 3u + /* max service record count */
+ ((p_reply) ? (*p_reply) : 0));
+
+ if (base_bytes > bytes_left) {
+ sdp_disconnect(p_ccb, SDP_INVALID_CONT_STATE);
+ return;
+ }
+
+ bytes_left -= base_bytes;
+
+ /* Build the UID sequence. */
#if (SDP_BROWSE_PLUS == TRUE)
p = sdpu_build_uuid_seq(p, 1,
- &p_ccb->p_db->uuid_filters[p_ccb->cur_uuid_idx]);
+ &p_ccb->p_db->uuid_filters[p_ccb->cur_uuid_idx],
+ bytes_left);
#else
p = sdpu_build_uuid_seq(p, p_ccb->p_db->num_uuid_filters,
- p_ccb->p_db->uuid_filters);
+ p_ccb->p_db->uuid_filters, bytes_left);
#endif
/* Max attribute byte count */

View file

@ -1,82 +0,0 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Brian Delwiche <delwiche@google.com>
Date: Tue, 21 Mar 2023 22:39:16 +0000
Subject: [PATCH] Revert "Revert "Fix wrong BR/EDR link key downgrades
(P_256->P_192)""
This reverts commit d733c86cbc06ce0ec72216b9d41e172d1939c46f.
Function btm_sec_encrypt_change() is called at most places
with argument "encr_enable" treated as bool and not as per
(tHCI_ENCRYPT_MODE = 0/1/2) expected by the function. The
function has special handling for "encr_enable=1" to downgrade
the link key type for BR/EDR case. This gets executed even
when the caller/context did not mean/expect so. It appears
this handling in btm_sec_encrypt_change() is not necessary and
is removed by this commit to prevent accidental execution of it.
Test: Verified re-pairing with an iPhone works fine now
Issue Reproduction Steps:
1. Enable Bluetooth Hotspot on Android device (DUT).
2. Pair and connect an iPhone to DUT.
3. Forget this pairing on DUT.
4. On iPhone settings, click on old DUT's paired entry to connect.
5. iPhone notifies to click 'Forget Device' and try fresh pairing.
6. On iPhone, after doing 'Forget Device', discover DUT again.
7. Attempt pairing to DUT by clicking on discovered DUT entry.
Pairing will be unsuccessful.
Issue Cause:
During re-pairing, DUT is seen to downgrade
BR/EDR link key unexpectedly from link key type 0x8
(BTM_LKEY_TYPE_AUTH_COMB_P_256) to 0x5 (BTM_LKEY_TYPE_AUTH_COMB).
Log snippet (re-pairing time):
btm_sec_link_key_notification set new_encr_key_256 to 1
btif_dm_auth_cmpl_evt: Storing link key. key_type=0x8, bond_type=1
btm_sec_encrypt_change new_encr_key_256 is 1
--On DUT, HCI_Encryption_Key_Refresh_Complete event noticed---
btm_sec_encrypt_change new_encr_key_256 is 0
updated link key type to 5
btif_dm_auth_cmpl_evt: Storing link key. key_type=0x5, bond_type=1
This is a backport of the following patch: aosp/1890096
Bug: 258834033
Reason for revert: Reinstate original change for QPR
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:56891eedc68c86b40977191dad28d65ebf86a94f)
Merged-In: Iba0c220b82bcf6b15368762b7052a3987ccbc0c6
Change-Id: Iba0c220b82bcf6b15368762b7052a3987ccbc0c6
---
stack/btm/btm_sec.cc | 16 ----------------
1 file changed, 16 deletions(-)
diff --git a/stack/btm/btm_sec.cc b/stack/btm/btm_sec.cc
index 277e01dc1..3ba1a6023 100644
--- a/stack/btm/btm_sec.cc
+++ b/stack/btm/btm_sec.cc
@@ -4035,22 +4035,6 @@ void btm_sec_encrypt_change(uint16_t handle, uint8_t status,
SMP_BR_PairWith(p_dev_rec->bd_addr);
}
}
- } else {
- // BR/EDR is successfully encrypted. Correct LK type if needed
- // (BR/EDR LK derived from LE LTK was used for encryption)
- if ((encr_enable == 1) && /* encryption is ON for SSP */
- /* LK type is for BR/EDR SC */
- (p_dev_rec->link_key_type == BTM_LKEY_TYPE_UNAUTH_COMB_P_256 ||
- p_dev_rec->link_key_type == BTM_LKEY_TYPE_AUTH_COMB_P_256)) {
- if (p_dev_rec->link_key_type == BTM_LKEY_TYPE_UNAUTH_COMB_P_256)
- p_dev_rec->link_key_type = BTM_LKEY_TYPE_UNAUTH_COMB;
- else /* BTM_LKEY_TYPE_AUTH_COMB_P_256 */
- p_dev_rec->link_key_type = BTM_LKEY_TYPE_AUTH_COMB;
-
- BTM_TRACE_DEBUG("updated link key type to %d",
- p_dev_rec->link_key_type);
- btm_send_link_key_notif(p_dev_rec);
- }
}
}