diff mbox series

[BlueZ] a2dp: fix setup->err use-after-free

Message ID 154e1a604eb8c3d924699489da72ea905915fb88.1710614196.git.pav@iki.fi (mailing list archive)
State Accepted
Commit c04b96dda5ce1bbb07a72b7ffa5ad1786ccffe47
Headers show
Series [BlueZ] a2dp: fix setup->err use-after-free | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch warning WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line) #98: Address 0x7f15ee5189c0 is located in stack of thread T0 at offset 64 in frame /github/workspace/src/src/13594478.patch total: 0 errors, 1 warnings, 168 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/13594478.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.
tedd_an/GitLint fail WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search 24: B1 Line exceeds max length (85>80): " [64, 72) 'err' (line 3057) <== Memory access at offset 64 is inside this variable"
tedd_an/BuildEll success Build ELL PASS
tedd_an/BluezMake success Bluez Make PASS

Commit Message

Pauli Virtanen March 16, 2024, 6:36 p.m. UTC
setup->err is set to values that either are on stack of avdtp.c
routines, obtained from callbacks, or allocated on heap. This is
inconsistent, and use-after-free in some cases.

Fix by always allocating setup->err ourselves, copying any values
obtained from callbacks.  Add setup_error_set/init and do all setup->err
manipulation via them.

Fixes crash:

==994225==ERROR: AddressSanitizer: stack-use-after-return
READ of size 1 at 0x7f15ee5189c0 thread T0
    #0 0x445724 in avdtp_error_category profiles/audio/avdtp.c:657
    #1 0x41e59e in error_to_errno profiles/audio/a2dp.c:303
    #2 0x42bb23 in a2dp_reconfigure profiles/audio/a2dp.c:1336
    #3 0x7f15f1512798 in g_timeout_dispatch
    ...
Address 0x7f15ee5189c0 is located in stack of thread T0 at offset 64 in frame
    #0 0x466b76 in avdtp_parse_rej profiles/audio/avdtp.c:3056
  This frame has 2 object(s):
    [48, 49) 'acp_seid' (line 3058)
    [64, 72) 'err' (line 3057) <== Memory access at offset 64 is inside this variable
---
 profiles/audio/a2dp.c | 68 +++++++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 29 deletions(-)

Comments

patchwork-bot+bluetooth@kernel.org March 20, 2024, 9:30 a.m. UTC | #1
Hello:

This patch was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Sat, 16 Mar 2024 20:36:38 +0200 you wrote:
> setup->err is set to values that either are on stack of avdtp.c
> routines, obtained from callbacks, or allocated on heap. This is
> inconsistent, and use-after-free in some cases.
> 
> Fix by always allocating setup->err ourselves, copying any values
> obtained from callbacks.  Add setup_error_set/init and do all setup->err
> manipulation via them.
> 
> [...]

Here is the summary with links:
  - [BlueZ] a2dp: fix setup->err use-after-free
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=c04b96dda5ce

You are awesome, thank you!
diff mbox series

Patch

diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
index b43161a13..a3c294bc3 100644
--- a/profiles/audio/a2dp.c
+++ b/profiles/audio/a2dp.c
@@ -222,6 +222,7 @@  static void setup_free(struct a2dp_setup *s)
 		avdtp_unref(s->session);
 	g_slist_free_full(s->cb, g_free);
 	g_slist_free_full(s->caps, g_free);
+	g_free(s->err);
 	g_free(s);
 }
 
@@ -270,17 +271,34 @@  static void setup_cb_free(struct a2dp_setup_cb *cb)
 	g_free(cb);
 }
 
+static void setup_error_set(struct a2dp_setup *setup, struct avdtp_error *err)
+{
+	if (!err) {
+		g_free(setup->err);
+		setup->err = NULL;
+	} else {
+		if (!setup->err)
+			setup->err = g_new0(struct avdtp_error, 1);
+		memcpy(setup->err, err, sizeof(struct avdtp_error));
+	}
+}
+
+static void setup_error_init(struct a2dp_setup *setup, uint8_t type, int id)
+{
+	struct avdtp_error err;
+
+	avdtp_error_init(&err, type, id);
+	setup_error_set(setup, &err);
+}
+
 static void finalize_setup_errno(struct a2dp_setup *s, int err,
 					GSourceFunc cb1, ...)
 {
 	GSourceFunc finalize;
 	va_list args;
-	struct avdtp_error avdtp_err;
 
-	if (err < 0) {
-		avdtp_error_init(&avdtp_err, AVDTP_ERRNO, -err);
-		s->err = &avdtp_err;
-	}
+	if (err < 0)
+		setup_error_init(s, AVDTP_ERRNO, -err);
 
 	va_start(args, cb1);
 	finalize = cb1;
@@ -576,10 +594,7 @@  done:
 
 	finalize_config(setup);
 
-	if (setup->err) {
-		g_free(setup->err);
-		setup->err = NULL;
-	}
+	setup_error_set(setup, NULL);
 
 	setup_unref(setup);
 
@@ -588,11 +603,9 @@  done:
 
 static void endpoint_setconf_cb(struct a2dp_setup *setup, gboolean ret)
 {
-	if (ret == FALSE) {
-		setup->err = g_new(struct avdtp_error, 1);
-		avdtp_error_init(setup->err, AVDTP_MEDIA_CODEC,
+	if (ret == FALSE)
+		setup_error_init(setup, AVDTP_MEDIA_CODEC,
 					AVDTP_UNSUPPORTED_CONFIGURATION);
-	}
 
 	auto_config(setup);
 	setup_unref(setup);
@@ -671,8 +684,7 @@  static gboolean endpoint_setconf_ind(struct avdtp *session,
 
 		if (cap->category == AVDTP_DELAY_REPORTING &&
 					!a2dp_sep->delay_reporting) {
-			setup->err = g_new(struct avdtp_error, 1);
-			avdtp_error_init(setup->err, AVDTP_DELAY_REPORTING,
+			setup_error_init(setup, AVDTP_DELAY_REPORTING,
 					AVDTP_UNSUPPORTED_CONFIGURATION);
 			goto done;
 		}
@@ -683,8 +695,7 @@  static gboolean endpoint_setconf_ind(struct avdtp *session,
 		codec = (struct avdtp_media_codec_capability *) cap->data;
 
 		if (codec->media_codec_type != a2dp_sep->codec) {
-			setup->err = g_new(struct avdtp_error, 1);
-			avdtp_error_init(setup->err, AVDTP_MEDIA_CODEC,
+			setup_error_init(setup, AVDTP_MEDIA_CODEC,
 					AVDTP_UNSUPPORTED_CONFIGURATION);
 			goto done;
 		}
@@ -704,10 +715,9 @@  static gboolean endpoint_setconf_ind(struct avdtp *session,
 			return TRUE;
 		}
 
-		setup_unref(setup);
-		setup->err = g_new(struct avdtp_error, 1);
-		avdtp_error_init(setup->err, AVDTP_MEDIA_CODEC,
+		setup_error_init(setup, AVDTP_MEDIA_CODEC,
 					AVDTP_UNSUPPORTED_CONFIGURATION);
+		setup_unref(setup);
 		break;
 	}
 
@@ -886,7 +896,7 @@  static void invalidate_remote_cache(struct a2dp_setup *setup,
 		/* Set error to -EAGAIN so the likes of policy plugin can
 		 * reattempt to connect.
 		 */
-		avdtp_error_init(setup->err, AVDTP_ERRNO, -EAGAIN);
+		setup_error_init(setup, AVDTP_ERRNO, -EAGAIN);
 	}
 }
 
@@ -910,10 +920,10 @@  static void setconf_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
 	if (err) {
 		if (setup) {
 			setup_ref(setup);
-			setup->err = err;
+			setup_error_set(setup, err);
 			invalidate_remote_cache(setup, err);
 			finalize_config(setup);
-			setup->err = NULL;
+			setup_error_set(setup, NULL);
 			setup_unref(setup);
 		}
 
@@ -1116,7 +1126,7 @@  static void open_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
 
 	if (err) {
 		setup->stream = NULL;
-		setup->err = err;
+		setup_error_set(setup, err);
 		if (setup->start)
 			finalize_resume(setup);
 	} else if (setup->chan)
@@ -1191,7 +1201,7 @@  static void start_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
 
 	if (err) {
 		setup->stream = NULL;
-		setup->err = err;
+		setup_error_set(setup, err);
 	}
 
 	finalize_resume(setup);
@@ -1270,7 +1280,7 @@  static void suspend_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
 
 	if (err) {
 		setup->stream = NULL;
-		setup->err = err;
+		setup_error_set(setup, err);
 	}
 
 	finalize_suspend(setup);
@@ -1418,7 +1428,7 @@  static void close_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
 
 	if (err) {
 		setup->stream = NULL;
-		setup->err = err;
+		setup_error_set(setup, err);
 		finalize_config(setup);
 		return;
 	}
@@ -1528,7 +1538,7 @@  static void reconf_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
 
 	if (err) {
 		setup->stream = NULL;
-		setup->err = err;
+		setup_error_set(setup, err);
 	}
 
 	finalize_config(setup);
@@ -2885,7 +2895,7 @@  static void discover_cb(struct avdtp *session, GSList *seps,
 
 	setup->seps = seps;
 	if (err)
-		setup->err = err;
+		setup_error_set(setup, err);
 
 	if (!err) {
 		g_slist_foreach(seps, foreach_register_remote_sep, setup->chan);