commit 17044ccf3a2858633cad8f87926e752edfe0d8d8
author Hui Peng <phui@google.com> Tue Aug 29 12:05:45 2023 -0700
committer Android Build Coastguard Worker <android-build-coastguard-worker@google.com> Wed Dec 06 22:44:33 2023 +0000
tree d27f2e35f0abf8093003964eef36ec811f500773
parent 1e6e0b5ff2a72780c8f0cc71c04e6504d627bc21

Fix an OOB write bug in attp_build_value_cmd

Bug: 295887535
Test: m com.android.btservices
Ignore-AOSP-First: security
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:b927f3fb660dafaf97b2fa0398353a8c39125efc)
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:a0d4425c3964f99f589d449deed2f1bbe520218c)
Merged-In: Ie16251c3a2b7c0f807ecb53bbf125d1e8c276e48
Change-Id: Ie16251c3a2b7c0f807ecb53bbf125d1e8c276e48

system/stack/gatt/att_protocol.cc

diff --git a/system/stack/gatt/att_protocol.cc b/system/stack/gatt/att_protocol.cc
index 29ceec2..f055fb8 100644
--- a/system/stack/gatt/att_protocol.cc
+++ b/system/stack/gatt/att_protocol.cc
@@ -286,46 +286,79 @@
 static BT_HDR* attp_build_value_cmd(uint16_t payload_size, uint8_t op_code,
                                     uint16_t handle, uint16_t offset,
                                     uint16_t len, uint8_t* p_data) {
-  uint8_t *p, *pp, pair_len, *p_pair_len;
+  uint8_t *p, *pp, *p_pair_len;
+  size_t pair_len;
+  size_t size_now = 1;
+
+  #define CHECK_SIZE() do {                      \
+    if (size_now > payload_size) {               \
+      LOG(ERROR) << "payload size too small";    \
+      osi_free(p_buf);                           \
+      return nullptr;                            \
+    }                                            \
+  } while (false)
+
   BT_HDR* p_buf =
       (BT_HDR*)osi_malloc(sizeof(BT_HDR) + payload_size + L2CAP_MIN_OFFSET);
 
   p = pp = (uint8_t*)(p_buf + 1) + L2CAP_MIN_OFFSET;
+
+  CHECK_SIZE();
   UINT8_TO_STREAM(p, op_code);
   p_buf->offset = L2CAP_MIN_OFFSET;
-  p_buf->len = 1;
 
   if (op_code == GATT_RSP_READ_BY_TYPE) {
     p_pair_len = p;
     pair_len = len + 2;
-    UINT8_TO_STREAM(p, pair_len);
-    p_buf->len += 1;
+    size_now += 1;
+    CHECK_SIZE();
+    // this field will be backfilled in the end of this function
   }
+
   if (op_code != GATT_RSP_READ_BLOB && op_code != GATT_RSP_READ) {
+    size_now += 2;
+    CHECK_SIZE();
     UINT16_TO_STREAM(p, handle);
-    p_buf->len += 2;
   }
 
   if (op_code == GATT_REQ_PREPARE_WRITE || op_code == GATT_RSP_PREPARE_WRITE) {
+    size_now += 2;
+    CHECK_SIZE();
     UINT16_TO_STREAM(p, offset);
-    p_buf->len += 2;
   }
 
-  if (len > 0 && p_data != NULL) {
+  if (len > 0 && p_data != NULL && payload_size > size_now) {
     /* ensure data not exceed MTU size */
-    if (payload_size - p_buf->len < len) {
-      len = payload_size - p_buf->len;
+    if (payload_size - size_now < len) {
+      len = payload_size - size_now;
       /* update handle value pair length */
-      if (op_code == GATT_RSP_READ_BY_TYPE) *p_pair_len = (len + 2);
+      if (op_code == GATT_RSP_READ_BY_TYPE) {
+        pair_len = (len + 2);
+      }
 
       LOG(WARNING) << StringPrintf(
           "attribute value too long, to be truncated to %d", len);
     }
 
+    size_now += len;
+    CHECK_SIZE();
     ARRAY_TO_STREAM(p, p_data, len);
-    p_buf->len += len;
   }
 
+  // backfill pair len field
+  if (op_code == GATT_RSP_READ_BY_TYPE) {
+    if (pair_len > UINT8_MAX) {
+      LOG(ERROR) << "pair_len greater than" << UINT8_MAX;
+      osi_free(p_buf);
+      return nullptr;
+    }
+
+    *p_pair_len = (uint8_t) pair_len;
+  }
+
+  #undef CHECK_SIZE
+
+  p_buf->len = (uint16_t) size_now;
   return p_buf;
 }