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(-)
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);
--
2.44.0
Hello:
This patch was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <[email protected]>:
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html