[BlueZ,v2] shared/gatt-db: Fix munmap_chunk invalid pointer

Commit Message

Frédéric Danis Jan. 5, 2024, 9:17 a.m. UTC

PTS test GATT/CL/GAD/BV-03-C published a service starting at handle 0xfffd
and ending at 0xffff.
This resets the next_handle to 0 in gatt_db_insert_service() instead of
setting it to 0x10000. Other services are added later.
This could end-up by a crash in db_hash_update() if not enough space has
been allocated for hash.iov and some entries are overwritten.
---
v1 -> v2: Replace next_handle by last_handle
          Check empty db using gatt_db_isempty(db) instead of
            next_handle == 0
          Add robustness unit test to check that gatt_db_get_hash()
            doesn't crash
---
 src/shared/gatt-db.c | 18 +++++-----
 unit/test-gatt.c     | 78 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 86 insertions(+), 10 deletions(-)

Comments

[e-mail] Jan. 5, 2024, 10:29 a.m. UTC | #1

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=814595

---Test result---

Test Summary:
CheckPatch                    FAIL      0.83 seconds
GitLint                       PASS      0.32 seconds
BuildEll                      PASS      24.16 seconds
BluezMake                     PASS      697.41 seconds
MakeCheck                     PASS      11.58 seconds
MakeDistcheck                 PASS      160.25 seconds
CheckValgrind                 PASS      222.11 seconds
CheckSmatch                   PASS      326.66 seconds
bluezmakeextell               PASS      106.59 seconds
IncrementalBuild              PASS      663.30 seconds
ScanBuild                     PASS      935.33 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ,v2] shared/gatt-db: Fix munmap_chunk invalid pointer
WARNING:LONG_LINE: line length of 81 exceeds 80 columns
#105: FILE: src/shared/gatt-db.c:366:
+	bt_crypto_gatt_hash(db->crypto, hash.iov, db->last_handle + 1, db->hash);

WARNING:LONG_LINE_COMMENT: line length of 89 exceeds 80 columns
#218: FILE: unit/test-gatt.c:2413:
+	/* test that gatt_db_get_hash is able to manage unordered db and doesn't crash */

/github/workspace/src/src/13511878.patch total: 0 errors, 2 warnings, 163 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13511878.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.




---
Regards,
Linux Bluetooth

Patch

diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 676f963ec..27d588677 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -58,7 +58,7 @@  struct gatt_db {
 	struct bt_crypto *crypto;
 	uint8_t hash[16];
 	unsigned int hash_id;
-	uint16_t next_handle;
+	uint16_t last_handle;
 	struct queue *services;
 
 	struct queue *notify_list;
@@ -255,7 +255,7 @@  struct gatt_db *gatt_db_new(void)
 	db->crypto = bt_crypto_new();
 	db->services = queue_new();
 	db->notify_list = queue_new();
-	db->next_handle = 0x0001;
+	db->last_handle = 0x0000;
 
 	return gatt_db_ref(db);
 }
@@ -356,14 +356,14 @@  static bool db_hash_update(void *user_data)
 
 	db->hash_id = 0;
 
-	if (!db->next_handle)
+	if (gatt_db_isempty(db))
 		return false;
 
-	hash.iov = new0(struct iovec, db->next_handle);
+	hash.iov = new0(struct iovec, db->last_handle + 1);
 	hash.i = 0;
 
 	gatt_db_foreach_service(db, NULL, service_gen_hash_m, &hash);
-	bt_crypto_gatt_hash(db->crypto, hash.iov, db->next_handle, db->hash);
+	bt_crypto_gatt_hash(db->crypto, hash.iov, db->last_handle + 1, db->hash);
 
 	for (i = 0; i < hash.i; i++)
 		free(hash.iov[i].iov_base);
@@ -624,7 +624,7 @@  bool gatt_db_clear_range(struct gatt_db *db, uint16_t start_handle,
 
 done:
 	if (gatt_db_isempty(db))
-		db->next_handle = 0;
+		db->last_handle = 0;
 
 	return true;
 }
@@ -700,7 +700,7 @@  struct gatt_db_attribute *gatt_db_insert_service(struct gatt_db *db,
 		return NULL;
 
 	if (!handle)
-		handle = db->next_handle;
+		handle = db->last_handle + 1;
 
 	if (num_handles < 1 || (handle + num_handles - 1) > UINT16_MAX)
 		return NULL;
@@ -747,8 +747,8 @@  struct gatt_db_attribute *gatt_db_insert_service(struct gatt_db *db,
 	service->attributes[0]->handle = handle;
 	service->num_handles = num_handles;
 
-	/* Fast-forward next_handle if the new service was added to the end */
-	db->next_handle = MAX(handle + num_handles, db->next_handle);
+	/* Fast-forward last_handle if the new service was added to the end */
+	db->last_handle = MAX(handle + num_handles - 1, db->last_handle);
 
 	return service->attributes[0];
 
diff --git a/unit/test-gatt.c b/unit/test-gatt.c
index f92d860c4..a8e28e2ce 100644
--- a/unit/test-gatt.c
+++ b/unit/test-gatt.c
@@ -1908,6 +1908,67 @@  static struct gatt_db *make_test_spec_large_db_1(void)
 	return make_db(specs);
 }
 
+/*
+ * Defined Test database 3:
+ * Tiny database fits into a single minimum sized-pdu with services unordered.
+ * Satisfies requirements:
+ * 5. At least one characteristic at the MAX handle
+ * 7. at least one service uuid with multiple instances
+ * 8. Some simple services, some with included services
+ * 9. an instance where handle of included service comes before the including
+ * service
+ * 11. Simple characteristics (no desc) and complex characteristics
+ *     (multiple descriptors)
+ * 12. Instances of complex chars with 16-bit and 128-bit uuids
+ *     (although not in scrambled order)
+ */
+
+static struct gatt_db *make_test_spec_unordered_db(void)
+{
+	const struct att_handle_spec specs[] = {
+		SECONDARY_SERVICE(0x0003, DEVICE_INFORMATION_UUID, 16),
+		CHARACTERISTIC_STR(GATT_CHARAC_MANUFACTURER_NAME_STRING,
+						BT_ATT_PERM_READ |
+						BT_ATT_PERM_WRITE,
+						BT_GATT_CHRC_PROP_READ |
+						BT_GATT_CHRC_PROP_NOTIFY |
+						BT_GATT_CHRC_PROP_INDICATE |
+						BT_GATT_CHRC_PROP_EXT_PROP,
+						"BlueZ"),
+		DESCRIPTOR(GATT_CLIENT_CHARAC_CFG_UUID, BT_ATT_PERM_READ |
+						BT_ATT_PERM_WRITE, 0x00, 0x00),
+		DESCRIPTOR_STR(GATT_CHARAC_USER_DESC_UUID, BT_ATT_PERM_READ,
+							"Manufacturer Name"),
+		DESCRIPTOR(GATT_CHARAC_EXT_PROPER_UUID, BT_ATT_PERM_READ, 0x01,
+									0x00),
+		CHARACTERISTIC_STR(GATT_CHARAC_SOFTWARE_REVISION_STRING,
+						BT_ATT_PERM_READ,
+						BT_GATT_CHRC_PROP_READ |
+						BT_GATT_CHRC_PROP_INDICATE,
+						"5.59"),
+		DESCRIPTOR(GATT_CLIENT_CHARAC_CFG_UUID, BT_ATT_PERM_READ
+			| BT_ATT_PERM_WRITE, 0x00, 0x00),
+
+		PRIMARY_SERVICE(0xFFFF - 9 + 1, GAP_UUID, 9),
+		INCLUDE(0x0003),
+		CHARACTERISTIC_STR(GATT_CHARAC_DEVICE_NAME, BT_ATT_PERM_READ,
+							BT_GATT_CHRC_PROP_READ,
+							"BlueZ Unit Tester"),
+		CHARACTERISTIC(0000B009-0000-0000-0123-456789abcdef,
+					BT_ATT_PERM_READ | BT_ATT_PERM_WRITE,
+					BT_GATT_CHRC_PROP_READ |
+					BT_GATT_CHRC_PROP_EXT_PROP, 0x09),
+		DESCRIPTOR(GATT_CHARAC_EXT_PROPER_UUID, BT_ATT_PERM_READ, 0x01,
+									0x00),
+		CHARACTERISTIC(GATT_CHARAC_APPEARANCE, BT_ATT_PERM_READ,
+					BT_GATT_CHRC_PROP_READ, 0x00, 0x00),
+		PRIMARY_SERVICE(0x0001, DEVICE_INFORMATION_UUID, 1),
+		{ }
+	};
+
+	return make_db(specs);
+}
+
 static void test_client(gconstpointer data)
 {
 	create_context(512, data);
@@ -2345,10 +2406,20 @@  static const struct test_step test_indication_server_1 = {
 	.length = 0x03,
 };
 
+static void test_hash_db(gconstpointer data)
+{
+	struct context *context = create_context(512, data);
+
+	/* test that gatt_db_get_hash is able to manage unordered db and doesn't crash */
+	gatt_db_get_hash(context->server_db);
+
+	context_quit(context);
+}
+
 int main(int argc, char *argv[])
 {
 	struct gatt_db *service_db_1, *service_db_2, *service_db_3;
-	struct gatt_db *ts_small_db, *ts_large_db_1;
+	struct gatt_db *ts_small_db, *ts_large_db_1, *ts_unordered_db;
 
 	tester_init(&argc, &argv);
 
@@ -2357,6 +2428,7 @@  int main(int argc, char *argv[])
 	service_db_3 = make_service_data_3_db();
 	ts_small_db = make_test_spec_small_db();
 	ts_large_db_1 = make_test_spec_large_db_1();
+	ts_unordered_db = make_test_spec_unordered_db();
 
 	/*
 	 * Server Configuration
@@ -4487,5 +4559,9 @@  int main(int argc, char *argv[])
 			raw_pdu(0xff, 0x00),
 			raw_pdu());
 
+	define_test_server("/robustness/hash-db",
+			test_hash_db, ts_unordered_db, NULL,
+			{});
+
 	return tester_run();
 }