[BlueZ] gatt: more fixes with cleanup on disconnect/timeout

Commit Message

Bernie Conrad Oct. 8, 2021, 12:16 a.m. UTC

From: Bernie Conrad <[e-mail]>

The changes in gatt-database.c fix a use after free that was introduced
after the last cleanup patch, ccc_new and write_new operations were not
being properly unregistered because they were not assigned a disconn_id. 

The changes in gatt-db add similar cleanup to pending reads/writes where
timeouts after a disconnect would cause a similar use after free with
already cleaned up resoureces, this adds a simple cb to set on a pending
read/write if a disconnect has occurred to skip the use.  

---
 src/gatt-database.c  |  4 ++--
 src/shared/gatt-db.c | 36 ++++++++++++++++++++++++++++++++++--
 2 files changed, 36 insertions(+), 4 deletions(-)

Comments

[e-mail] Oct. 8, 2021, 1:32 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=559591

---Test result---

Test Summary:
CheckPatch                    FAIL      0.74 seconds
GitLint                       FAIL      0.32 seconds
Prep - Setup ELL              PASS      43.52 seconds
Build - Prep                  PASS      0.22 seconds
Build - Configure             PASS      8.39 seconds
Build - Make                  PASS      186.80 seconds
Make Check                    PASS      8.86 seconds
Make Distcheck                PASS      219.70 seconds
Build w/ext ELL - Configure   PASS      7.52 seconds
Build w/ext ELL - Make        PASS      181.99 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script with rule in .checkpatch.conf
Output:
[BlueZ] gatt: more fixes with cleanup on disconnect/timeout
WARNING:LONG_LINE: line length of 90 exceeds 80 columns
#187: FILE: src/shared/gatt-db.c:1922:
+		p->disconn_id = bt_att_register_disconnect(att, pending_read_cb, p, NULL);

WARNING:LONG_LINE: line length of 91 exceeds 80 columns
#212: FILE: src/shared/gatt-db.c:2027:
+		p->disconn_id = bt_att_register_disconnect(att, pending_write_cb, p, NULL);

/github/workspace/src/12543903.patch total: 0 errors, 2 warnings, 107 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/12543903.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.


##############################
Test: GitLint - FAIL
Desc: Run gitlint with rule in .gitlint
Output:
[BlueZ] gatt: more fixes with cleanup on disconnect/timeout
7: B2 Line has trailing whitespace: "being properly unregistered because they were not assigned a disconn_id. "
12: B2 Line has trailing whitespace: "read/write if a disconnect has occurred to skip the use.  "




---
Regards,
Linux Bluetooth

Patch

diff --git a/src/gatt-database.c b/src/gatt-database.c
index 475e7873c..00647cf08 100644
--- a/src/gatt-database.c
+++ b/src/gatt-database.c
@@ -978,7 +978,7 @@  static struct pending_op *pending_ccc_new(struct bt_att *att,
 	op->attrib = attrib;
 	op->link_type = link_type;
 
-	bt_att_register_disconnect(att,
+	op->disconn_id = bt_att_register_disconnect(att,
 				   pending_disconnect_cb,
 				   op,
 				   NULL);
@@ -2418,7 +2418,7 @@  static struct pending_op *pending_write_new(struct bt_att *att,
 	op->prep_authorize = prep_authorize;
 	queue_push_tail(owner_queue, op);
 
-	bt_att_register_disconnect(att,
+	op->disconn_id = bt_att_register_disconnect(att,
 			    pending_disconnect_cb,
 			    op, NULL);
 
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 3a02289ce..c1e676607 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -77,17 +77,23 @@  struct attribute_notify {
 
 struct pending_read {
 	struct gatt_db_attribute *attrib;
+	struct bt_att *att;
 	unsigned int id;
 	unsigned int timeout_id;
 	gatt_db_attribute_read_t func;
+	bool disconn;
+	unsigned int disconn_id;
 	void *user_data;
 };
 
 struct pending_write {
 	struct gatt_db_attribute *attrib;
+	struct bt_att *att;
 	unsigned int id;
 	unsigned int timeout_id;
 	gatt_db_attribute_write_t func;
+	bool disconn;
+	unsigned int disconn_id;
 	void *user_data;
 };
 
@@ -139,8 +145,10 @@  static void pending_read_result(struct pending_read *p, int err,
 	if (p->timeout_id > 0)
 		timeout_remove(p->timeout_id);
 
-	p->func(p->attrib, err, data, length, p->user_data);
+	if (!p->disconn)
+		p->func(p->attrib, err, data, length, p->user_data);
 
+	bt_att_unregister_disconnect(p->att, p->disconn_id);
 	free(p);
 }
 
@@ -156,8 +164,10 @@  static void pending_write_result(struct pending_write *p, int err)
 	if (p->timeout_id > 0)
 		timeout_remove(p->timeout_id);
 
-	p->func(p->attrib, err, p->user_data);
+	if (!p->disconn)
+		p->func(p->attrib, err, p->user_data);
 
+	bt_att_unregister_disconnect(p->att, p->disconn_id);
 	free(p);
 }
 
@@ -1868,6 +1878,13 @@  bool gatt_db_attribute_set_fixed_length(struct gatt_db_attribute *attrib,
 	return true;
 }
 
+static void pending_read_cb(int err, void *user_data)
+{
+	struct pending_read *p = user_data;
+
+	p->disconn = 1;
+}
+
 bool gatt_db_attribute_read(struct gatt_db_attribute *attrib, uint16_t offset,
 				uint8_t opcode, struct bt_att *att,
 				gatt_db_attribute_read_t func, void *user_data)
@@ -1901,6 +1918,10 @@  bool gatt_db_attribute_read(struct gatt_db_attribute *attrib, uint16_t offset,
 		p->func = func;
 		p->user_data = user_data;
 
+		p->disconn = 0;
+		p->disconn_id = bt_att_register_disconnect(att, pending_read_cb, p, NULL);
+		p->att = att;
+
 		queue_push_tail(attrib->pending_reads, p);
 
 		attrib->read_func(attrib, p->id, offset, opcode, att,
@@ -1956,6 +1977,13 @@  static bool write_timeout(void *user_data)
 	return false;
 }
 
+static void pending_write_cb(int err, void *user_data)
+{
+	struct pending_write *p = user_data;
+
+	p->disconn = 1;
+}
+
 bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
 					const uint8_t *value, size_t len,
 					uint8_t opcode, struct bt_att *att,
@@ -1995,6 +2023,10 @@  bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
 		p->func = func;
 		p->user_data = user_data;
 
+		p->disconn = 0;
+		p->disconn_id = bt_att_register_disconnect(att, pending_write_cb, p, NULL);
+		p->att = att;
+
 		queue_push_tail(attrib->pending_writes, p);
 
 		attrib->write_func(attrib, p->id, offset, value, len, opcode,