[PATCH BlueZ 1/2] shared/mcp: fix crash on destroy after ATT gone

[PATCH BlueZ 1/2] shared/mcp: fix crash on destroy after ATT gone

[Pauli Virtanen]

Tie lifetime of bt_mcs_session to that of ATT disconnect watch.  The
disconnect handler is not necessarily called before ATT is freed, but
destroy is.

Fixes UAF of session->att in bt_mcs_unregister() -> session_destroy() ->
bt_att_unregister_disconnect() if ATT was freed first.
---
 src/shared/mcp.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/src/shared/mcp.c b/src/shared/mcp.c
index a25d7b5a2..0dd175c6d 100644
--- a/src/shared/mcp.c
+++ b/src/shared/mcp.c
@@ -566,19 +566,20 @@ static bool match_session_att(const void *data, const void *match_data)
 static void session_destroy(void *data)
 {
 	struct bt_mcs_session *session = data;
+	struct bt_mcs *mcs = session->mcs;
 
-	bt_att_unregister_disconnect(session->att, session->disconn_id);
+	if (mcs)
+		queue_remove(mcs->sessions, session);
 	queue_destroy(session->changed, NULL);
 	free(session);
 }
 
-static void session_disconnect(int err, void *user_data)
+static void session_remove(void *user_data)
 {
 	struct bt_mcs_session *session = user_data;
-	struct bt_mcs *mcs = session->mcs;
 
-	queue_remove(mcs->sessions, session);
-	session_destroy(session);
+	session->mcs = NULL;
+	bt_att_unregister_disconnect(session->att, session->disconn_id);
 }
 
 static struct bt_mcs_session *get_session(struct bt_mcs *mcs,
@@ -591,8 +592,8 @@ static struct bt_mcs_session *get_session(struct bt_mcs *mcs,
 		return session;
 
 	session = new0(struct bt_mcs_session, 1);
-	session->disconn_id = bt_att_register_disconnect(att,
-					session_disconnect, session, NULL);
+	session->disconn_id = bt_att_register_disconnect(att, NULL, session,
+							session_destroy);
 	if (!session->disconn_id) {
 		free(session);
 		return NULL;
@@ -1036,7 +1037,7 @@ void bt_mcs_unregister(struct bt_mcs *mcs)
 		servers = NULL;
 	}
 
-	queue_destroy(mcs->sessions, session_destroy);
+	queue_destroy(mcs->sessions, session_remove);
 
 	free(mcs);
 }
-- 
2.53.0

[PATCH BlueZ 2/2] shared/mcp: fix parsing of control point arguments

[Pauli Virtanen]

Fix parsing of control point argument lost in rebases.

Add tests for Move Relative command that need it:

MCS/SR/MCP/BV-12-C [Move Relative from Playing]
MCS/SR/MCP/BV-13-C [Move Relative from Paused]
MCS/SR/MCP/BV-14-C [Move Relative from Seeking]
MCS/SR/MCP/BV-75-C [Move Relative from Inactive]
---
 src/shared/mcp.c |   5 +++
 unit/test-mcp.c  | 101 ++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 104 insertions(+), 2 deletions(-)

diff --git a/src/shared/mcp.c b/src/shared/mcp.c
index 0dd175c6d..a954869b0 100644
--- a/src/shared/mcp.c
+++ b/src/shared/mcp.c
@@ -343,6 +343,11 @@ static void write_media_cp(struct gatt_db_attribute *attrib,
 		rsp.result = BT_MCS_RESULT_OP_NOT_SUPPORTED;
 		goto respond;
 	}
+	if (cmd->int32_arg && !util_iov_pull_le32(&iov, (uint32_t *)&arg)) {
+		ret = BT_ATT_ERROR_INVALID_ATTRIBUTE_VALUE_LEN;
+		rsp.op = 0;
+		goto respond;
+	}
 
 	DBG_MCS(mcs, "Command %s", cmd->name);
 
diff --git a/unit/test-mcp.c b/unit/test-mcp.c
index 7d922bb83..f98e7e72b 100644
--- a/unit/test-mcp.c
+++ b/unit/test-mcp.c
@@ -1812,17 +1812,48 @@ static int32_t sr_mcp_track_position(void *user_data)
 {
 	struct test_data *data = user_data;
 
-	return 71 - data->id;
+	return data->id;
 }
 
 static bool sr_mcp_set_track_position(void *user_data, int32_t value)
 {
 	struct test_data *data = user_data;
 
-	data->id = 71 - value;
+	data->id = value;
 	return true;
 }
 
+static bool sr_mcp_move_relative(void *user_data, int32_t offset)
+{
+	struct test_data *data = user_data;
+	int pos = data->id;
+
+	tester_debug("Move Relative %d (pos %d)", (int)offset, pos);
+
+	if (bt_mcs_get_media_state(data->mcs) == BT_MCS_STATE_INACTIVE)
+		return false;
+
+	pos += offset;
+	if (pos < 0)
+		pos = 0;
+	if (pos > 71)
+		pos = 71;
+
+	data->id = pos;
+
+	if (!data->step)
+		FAIL_TEST();
+	data->step--;
+
+	bt_mcs_changed(data->mcs, MCS_TRACK_POSITION_CHRC_UUID);
+	return true;
+}
+
+static int32_t sr_mcp_track_duration(void *user_data)
+{
+	return 71;
+}
+
 const struct bt_mcs_callback sr_mcp_mcs = {
 	.media_cp_op_supported = sr_mcp_media_cp_op_supported,
 	.play = sr_mcp_op_success,
@@ -1832,6 +1863,8 @@ const struct bt_mcs_callback sr_mcp_mcs = {
 	.stop = sr_mcp_op_success_inactive,
 	.track_position = sr_mcp_track_position,
 	.set_track_position = sr_mcp_set_track_position,
+	.track_duration = sr_mcp_track_duration,
+	.move_relative = sr_mcp_move_relative,
 	.debug = mcs_debug,
 };
 
@@ -1908,15 +1941,65 @@ MCS_SR_MCP_CFG(bv_11_c, BT_MCS_STATE_SEEKING);
 MCS_SR_MCP_CFG(bv_74_c, BT_MCS_STATE_INACTIVE);
 #define MCS_SR_MCP_BV_74_C SR_MCP_STOP_INACTIVE
 
+#define SR_MCP_MOVE_RELATIVE(initial) \
+	NOTIFY_CHRC(STATE, initial), \
+	READ_CHRC(TRACK_DUR, 0x47, 0x00, 0x00, 0x00), \
+	WRITE_NORESP_CHRC(CP, BT_MCS_CMD_MOVE_RELATIVE, 0xaf, 0xff, 0xff, \
+									0xff), \
+	NOTIFY_CHRC(TRACK_POS, 0x00, 0x00, 0x00, 0x00), \
+	NOTIFY_CHRC(CP, BT_MCS_CMD_MOVE_RELATIVE, 0x01), \
+	WRITE_NORESP_CHRC(CP, BT_MCS_CMD_MOVE_RELATIVE, 0x17, 0x00, 0x00, \
+									0x00), \
+	NOTIFY_CHRC(TRACK_POS, 0x17, 0x00, 0x00, 0x00), \
+	NOTIFY_CHRC(CP, BT_MCS_CMD_MOVE_RELATIVE, 0x01), \
+	WRITE_NORESP_CHRC(CP, BT_MCS_CMD_MOVE_RELATIVE, 0x30, 0x00, 0x00, \
+									0x00), \
+	NOTIFY_CHRC(TRACK_POS, 0x47, 0x00, 0x00, 0x00), \
+	NOTIFY_CHRC(CP, BT_MCS_CMD_MOVE_RELATIVE, 0x01)
+
+#define SR_MCP_MOVE_RELATIVE_INACTIVE \
+	NOTIFY_CHRC(STATE, initial), \
+	READ_CHRC(TRACK_DUR, 0x47, 0x00, 0x00, 0x00), \
+	WRITE_NORESP_CHRC(CP, BT_MCS_CMD_MOVE_RELATIVE, 0xaf, 0xff, 0xff, \
+									0xff), \
+	NOTIFY_CHRC(CP, BT_MCS_CMD_MOVE_RELATIVE, 0x03)
+
+MCS_SR_MCP_CFG(bv_12_c, BT_MCS_STATE_PLAYING);
+#define MCS_SR_MCP_BV_12_C SR_MCP_MOVE_RELATIVE(BT_MCS_STATE_PLAYING)
+
+MCS_SR_MCP_CFG(bv_13_c, BT_MCS_STATE_PAUSED);
+#define MCS_SR_MCP_BV_13_C SR_MCP_MOVE_RELATIVE(BT_MCS_STATE_PAUSED)
+
+MCS_SR_MCP_CFG(bv_14_c, BT_MCS_STATE_SEEKING);
+#define MCS_SR_MCP_BV_14_C SR_MCP_MOVE_RELATIVE(BT_MCS_STATE_SEEKING)
+
+MCS_SR_MCP_CFG(bv_75_c, BT_MCS_STATE_INACTIVE);
+#define MCS_SR_MCP_BV_75_C SR_MCP_MOVE_RELATIVE_INACTIVE
+
 static void test_sr_mcp(const void *user_data)
 {
 	struct test_data *data = (void *)user_data;
 
+	data->id = 13;
+
 	bt_mcs_set_media_state(data->mcs, data->cfg->state);
 	data->step++;
 	test_server(data);
 }
 
+static void test_sr_mcp_move_relative(const void *user_data)
+{
+	struct test_data *data = (void *)user_data;
+
+	data->id = 13;
+
+	bt_mcs_set_media_state(data->mcs, data->cfg->state);
+	data->step++;
+	if (data->cfg->state != BT_MCS_STATE_INACTIVE)
+		data->step += 2;
+	test_server(data);
+}
+
 static void testgroup_sr_mcp(void)
 {
 	/* Only the MCS tests. No point in GMCS as only svc uuid changes */
@@ -1955,6 +2038,20 @@ static void testgroup_sr_mcp(void)
 		test_setup_server, test_sr_mcp,
 		&cfg_mcs_sr_mcp_bv_74_c, MCS_SR_MCP_BV_74_C);
 
+	/* MCS.TS Sec 4.4.4 Move Relative */
+	define_test("MCS/SR/MCP/BV-12-C [Move Relative from Playing]",
+		test_setup_server, test_sr_mcp_move_relative,
+		&cfg_mcs_sr_mcp_bv_12_c, MCS_SR_MCP_BV_12_C);
+	define_test("MCS/SR/MCP/BV-13-C [Move Relative from Paused]",
+		test_setup_server, test_sr_mcp_move_relative,
+		&cfg_mcs_sr_mcp_bv_13_c, MCS_SR_MCP_BV_13_C);
+	define_test("MCS/SR/MCP/BV-14-C [Move Relative from Seeking]",
+		test_setup_server, test_sr_mcp_move_relative,
+		&cfg_mcs_sr_mcp_bv_14_c, MCS_SR_MCP_BV_14_C);
+	define_test("MCS/SR/MCP/BV-75-C [Move Relative from Inactive]",
+		test_setup_server, test_sr_mcp_move_relative,
+		&cfg_mcs_sr_mcp_bv_75_c, MCS_SR_MCP_BV_74_C);
+
 	/* TODO: other state transition tests. They largely test the profile
 	 * upper layer, so do not add much here.
 	 */
-- 
2.53.0

RE: [BlueZ,1/2] shared/mcp: fix crash on destroy after ATT gone

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1311 bytes --]

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=1077651

---Test result---

Test Summary:
CheckPatch                    PENDING   0.44 seconds
GitLint                       PENDING   0.44 seconds
BuildEll                      PASS      20.32 seconds
BluezMake                     PASS      647.48 seconds
MakeCheck                     PASS      18.63 seconds
MakeDistcheck                 PASS      243.38 seconds
CheckValgrind                 PASS      292.86 seconds
CheckSmatch                   PASS      345.96 seconds
bluezmakeextell               PASS      180.59 seconds
IncrementalBuild              PENDING   0.42 seconds
ScanBuild                     PASS      1010.00 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



https://github.com/bluez/bluez/pull/2020/checks

---
Regards,
Linux Bluetooth