[PATCH BlueZ 1/2] a2dp: disallow multiple SetConfiguration to same local SEP

[PATCH BlueZ 1/2] a2dp: disallow multiple SetConfiguration to same local SEP

[Pauli Virtanen]

Using the remote SEP SetConfiguration DBus API, it's possible to make
multiple remote endpoints use the same local SEP, if they are endpoints
from different connected devices. This is invalid: successful
configuration shall prevent a different device configuring the same SEP
(AVDTP v1.3 Sec. 5.3).  Moreover, this breaks the assumption in the
AVDTP code that each SEP has at most a single stream, and causes
misbehavior later on (subsequent transport acquires fail with EPERM).

Fix this by first checking the SEP is free before proceeding in the DBus
API call.  Also add a sanity check in avdtp_set_configuration, to reject
configuring an already configured SEP similarly as in avdtp_setconf_cmd.
---

Notes:
    E.g. trying to set the same codec for two simultaneously connected
    devices for the same adapter in Pulseaudio, causes the A2DP
    connection of the first device stop working, as its transport
    acquires start failing with EPERM. Disconnecting the first device
    also breaks the second device connection.
    This patch fixes it so that only the invalid SetConfiguration fails.

 profiles/audio/a2dp.c  | 5 +++++
 profiles/audio/avdtp.c | 3 +++
 2 files changed, 8 insertions(+)

diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
index 6f5b13711..f3e2cdd9e 100644
--- a/profiles/audio/a2dp.c
+++ b/profiles/audio/a2dp.c
@@ -1843,6 +1843,11 @@ static int a2dp_reconfig(struct a2dp_channel *chan, const char *sender,
 	GSList *l;
 	int err;
 
+	/* Check SEP not used by a different session */
+	if (lsep->stream && chan->session &&
+	    !avdtp_has_stream(chan->session, lsep->stream))
+		return -EBUSY;
+
 	setup = a2dp_setup_get(chan->session);
 	if (!setup)
 		return -ENOMEM;
diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
index da4114e0f..bc7afad81 100644
--- a/profiles/audio/avdtp.c
+++ b/profiles/audio/avdtp.c
@@ -3523,6 +3523,9 @@ int avdtp_set_configuration(struct avdtp *session,
 	if (!(lsep && rsep))
 		return -EINVAL;
 
+	if (lsep->stream)
+		return -EBUSY;
+
 	DBG("%p: int_seid=%u, acp_seid=%u", session,
 			lsep->info.seid, rsep->seid);
 
-- 
2.36.1

[PATCH BlueZ 2/2] a2dp: error return paths in a2dp_reconfig must free allocated setup

[Pauli Virtanen]

---
 profiles/audio/a2dp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
index f3e2cdd9e..276512208 100644
--- a/profiles/audio/a2dp.c
+++ b/profiles/audio/a2dp.c
@@ -1876,8 +1876,10 @@ static int a2dp_reconfig(struct a2dp_channel *chan, const char *sender,
 		if (tmp->stream) {
 			/* Only allow switching sep from the same sender */
 			if (strcmp(sender, tmp->endpoint->get_name(tmp,
-							tmp->user_data)))
-				return -EPERM;
+							tmp->user_data))) {
+				err = -EPERM;
+				goto fail;
+			}
 
 			/* Check if stream is for the channel */
 			if (!avdtp_has_stream(chan->session, tmp->stream))
-- 
2.36.1

RE: [BlueZ,1/2] a2dp: disallow multiple SetConfiguration to same local SEP

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 998 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=647442

---Test result---

Test Summary:
CheckPatch                    PASS      2.91 seconds
GitLint                       PASS      1.84 seconds
Prep - Setup ELL              PASS      43.36 seconds
Build - Prep                  PASS      0.69 seconds
Build - Configure             PASS      8.69 seconds
Build - Make                  PASS      1280.99 seconds
Make Check                    PASS      11.99 seconds
Make Check w/Valgrind         PASS      445.82 seconds
Make Distcheck                PASS      233.21 seconds
Build w/ext ELL - Configure   PASS      8.80 seconds
Build w/ext ELL - Make        PASS      1287.67 seconds
Incremental Build with patchesPASS      2641.51 seconds



---
Regards,
Linux Bluetooth

Re: [PATCH BlueZ 1/2] a2dp: disallow multiple SetConfiguration to same local SEP

[Luiz Augusto von Dentz]

Hi Pauli,

On Sun, Jun 5, 2022 at 9:47 PM Pauli Virtanen <pav@iki.fi> wrote:
>
> Using the remote SEP SetConfiguration DBus API, it's possible to make
> multiple remote endpoints use the same local SEP, if they are endpoints
> from different connected devices. This is invalid: successful
> configuration shall prevent a different device configuring the same SEP
> (AVDTP v1.3 Sec. 5.3).  Moreover, this breaks the assumption in the
> AVDTP code that each SEP has at most a single stream, and causes
> misbehavior later on (subsequent transport acquires fail with EPERM).

Not sure I follow I follow why it would be invalid for a stack to
enable connecting the same local SEP with different remote SEP, afaik
this depends only if the underline codec does support multiple
streams, as far I can remember the folks at BMW were actually the ones
proposing such a change back in the days so perhaps something broke
the proper support so we should be able to fix it. If, and only if,
the codec itself don't support multiple simultaneous stream then it
should reject the SetConfiguration by replying with an error.

> Fix this by first checking the SEP is free before proceeding in the DBus
> API call.  Also add a sanity check in avdtp_set_configuration, to reject
> configuring an already configured SEP similarly as in avdtp_setconf_cmd.
> ---
>
> Notes:
>     E.g. trying to set the same codec for two simultaneously connected
>     devices for the same adapter in Pulseaudio, causes the A2DP
>     connection of the first device stop working, as its transport
>     acquires start failing with EPERM. Disconnecting the first device
>     also breaks the second device connection.
>     This patch fixes it so that only the invalid SetConfiguration fails.
>
>  profiles/audio/a2dp.c  | 5 +++++
>  profiles/audio/avdtp.c | 3 +++
>  2 files changed, 8 insertions(+)
>
> diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> index 6f5b13711..f3e2cdd9e 100644
> --- a/profiles/audio/a2dp.c
> +++ b/profiles/audio/a2dp.c
> @@ -1843,6 +1843,11 @@ static int a2dp_reconfig(struct a2dp_channel *chan, const char *sender,
>         GSList *l;
>         int err;
>
> +       /* Check SEP not used by a different session */
> +       if (lsep->stream && chan->session &&
> +           !avdtp_has_stream(chan->session, lsep->stream))
> +               return -EBUSY;
> +
>         setup = a2dp_setup_get(chan->session);
>         if (!setup)
>                 return -ENOMEM;
> diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
> index da4114e0f..bc7afad81 100644
> --- a/profiles/audio/avdtp.c
> +++ b/profiles/audio/avdtp.c
> @@ -3523,6 +3523,9 @@ int avdtp_set_configuration(struct avdtp *session,
>         if (!(lsep && rsep))
>                 return -EINVAL;
>
> +       if (lsep->stream)
> +               return -EBUSY;
> +
>         DBG("%p: int_seid=%u, acp_seid=%u", session,
>                         lsep->info.seid, rsep->seid);
>
> --
> 2.36.1
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 1/2] a2dp: disallow multiple SetConfiguration to same local SEP

[Pauli Virtanen]

Hi Luiz,

7. kesäkuuta 2022 8.33.46 GMT+03:00 Luiz Augusto von Dentz <luiz.dentz@gmail.com> kirjoitti:
>Hi Pauli,
>
>On Sun, Jun 5, 2022 at 9:47 PM Pauli Virtanen <pav@iki.fi> wrote:
>>
>> Using the remote SEP SetConfiguration DBus API, it's possible to make
>> multiple remote endpoints use the same local SEP, if they are endpoints
>> from different connected devices. This is invalid: successful
>> configuration shall prevent a different device configuring the same SEP
>> (AVDTP v1.3 Sec. 5.3).  Moreover, this breaks the assumption in the
>> AVDTP code that each SEP has at most a single stream, and causes
>> misbehavior later on (subsequent transport acquires fail with EPERM).
>
>Not sure I follow I follow why it would be invalid for a stack to
>enable connecting the same local SEP with different remote SEP, afaik
>this depends only if the underline codec does support multiple
>streams, as far I can remember the folks at BMW were actually the ones
>proposing such a change back in the days so perhaps something broke
>the proper support so we should be able to fix it. If, and only if,
>the codec itself don't support multiple simultaneous stream then it
>should reject the SetConfiguration by replying with an error.

My understanding here derives just from AVDTP spec 5.3 stating that "On successful termination of the configuration procedure, resources in both Device A and Device B shall be allocated (locked), and neither SEP v in Device A nor SEP z in Device B could be configured for another stream connection e.g. by a third device." which seems to forbid it. Maybe this reading is not correct?

It however doesnt't work (see below), and the SelectConfiguration mechanism explicitly skips in-use local endpoints (see avdtp_find_remote_sep), so it appears current code is not consistent on whether to allow it or not.

Technically, the problem is that struct avdtp_local_sep has only the struct avdtp_stream pointer, which just gets overwritten on every SetConfiguration. This later prevents acquiring the transport for the stream whose pointer is not there. stream_free will also set the pointer to NULL, so disconnecting one of the two devices breaks the other.

In principle if this is intended to work, the streams could just be looked up from the streams list instead, and one could just remove the reference in avdtp_stream. Also things like the inuse flag etc. shouldn't be shared, so it seems it needs a bit more work.

Things work when connecting to different adapters, as then the lsep is not the same.


Best,
Pauli




>
>> Fix this by first checking the SEP is free before proceeding in the DBus
>> API call.  Also add a sanity check in avdtp_set_configuration, to reject
>> configuring an already configured SEP similarly as in avdtp_setconf_cmd.
>> ---
>>
>> Notes:
>>     E.g. trying to set the same codec for two simultaneously connected
>>     devices for the same adapter in Pulseaudio, causes the A2DP
>>     connection of the first device stop working, as its transport
>>     acquires start failing with EPERM. Disconnecting the first device
>>     also breaks the second device connection.
>>     This patch fixes it so that only the invalid SetConfiguration fails.
>>
>>  profiles/audio/a2dp.c  | 5 +++++
>>  profiles/audio/avdtp.c | 3 +++
>>  2 files changed, 8 insertions(+)
>>
>> diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
>> index 6f5b13711..f3e2cdd9e 100644
>> --- a/profiles/audio/a2dp.c
>> +++ b/profiles/audio/a2dp.c
>> @@ -1843,6 +1843,11 @@ static int a2dp_reconfig(struct a2dp_channel *chan, const char *sender,
>>         GSList *l;
>>         int err;
>>
>> +       /* Check SEP not used by a different session */
>> +       if (lsep->stream && chan->session &&
>> +           !avdtp_has_stream(chan->session, lsep->stream))
>> +               return -EBUSY;
>> +
>>         setup = a2dp_setup_get(chan->session);
>>         if (!setup)
>>                 return -ENOMEM;
>> diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
>> index da4114e0f..bc7afad81 100644
>> --- a/profiles/audio/avdtp.c
>> +++ b/profiles/audio/avdtp.c
>> @@ -3523,6 +3523,9 @@ int avdtp_set_configuration(struct avdtp *session,
>>         if (!(lsep && rsep))
>>                 return -EINVAL;
>>
>> +       if (lsep->stream)
>> +               return -EBUSY;
>> +
>>         DBG("%p: int_seid=%u, acp_seid=%u", session,
>>                         lsep->info.seid, rsep->seid);
>>
>> --
>> 2.36.1
>>
>
>

Re: [PATCH BlueZ 1/2] a2dp: disallow multiple SetConfiguration to same local SEP

[Luiz Augusto von Dentz]

Hi Pauli,

On Tue, Jun 7, 2022 at 8:53 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi Luiz,
>
> 7. kesäkuuta 2022 8.33.46 GMT+03:00 Luiz Augusto von Dentz <luiz.dentz@gmail.com> kirjoitti:
> >Hi Pauli,
> >
> >On Sun, Jun 5, 2022 at 9:47 PM Pauli Virtanen <pav@iki.fi> wrote:
> >>
> >> Using the remote SEP SetConfiguration DBus API, it's possible to make
> >> multiple remote endpoints use the same local SEP, if they are endpoints
> >> from different connected devices. This is invalid: successful
> >> configuration shall prevent a different device configuring the same SEP
> >> (AVDTP v1.3 Sec. 5.3).  Moreover, this breaks the assumption in the
> >> AVDTP code that each SEP has at most a single stream, and causes
> >> misbehavior later on (subsequent transport acquires fail with EPERM).
> >
> >Not sure I follow I follow why it would be invalid for a stack to
> >enable connecting the same local SEP with different remote SEP, afaik
> >this depends only if the underline codec does support multiple
> >streams, as far I can remember the folks at BMW were actually the ones
> >proposing such a change back in the days so perhaps something broke
> >the proper support so we should be able to fix it. If, and only if,
> >the codec itself don't support multiple simultaneous stream then it
> >should reject the SetConfiguration by replying with an error.
>
> My understanding here derives just from AVDTP spec 5.3 stating that "On successful termination of the configuration procedure, resources in both Device A and Device B shall be allocated (locked), and neither SEP v in Device A nor SEP z in Device B could be configured for another stream connection e.g. by a third device." which seems to forbid it. Maybe this reading is not correct?

Need to check if the text is just information related to how to set
in_use flag or not, afaik there is no test in the testing
specification that do enforce setting the in_use flag since that would
require a third controller in order to do that.

> It however doesnt't work (see below), and the SelectConfiguration mechanism explicitly skips in-use local endpoints (see avdtp_find_remote_sep), so it appears current code is not consistent on whether to allow it or not.

Yep, that would probably need to be changed if we do want to be able
to reuse the same local SEP.

> Technically, the problem is that struct avdtp_local_sep has only the struct avdtp_stream pointer, which just gets overwritten on every SetConfiguration. This later prevents acquiring the transport for the stream whose pointer is not there. stream_free will also set the pointer to NULL, so disconnecting one of the two devices breaks the other.

This should be fixed asap since we are probably leaking the data, so
perhaps we can apply these changes and just add a comment that needs
changes if we want to allow the local SEP to be reused.

> In principle if this is intended to work, the streams could just be looked up from the streams list instead, and one could just remove the reference in avdtp_stream. Also things like the inuse flag etc. shouldn't be shared, so it seems it needs a bit more work.
>
> Things work when connecting to different adapters, as then the lsep is not the same.
>
>
> Best,
> Pauli
>
>
>
>
> >
> >> Fix this by first checking the SEP is free before proceeding in the DBus
> >> API call.  Also add a sanity check in avdtp_set_configuration, to reject
> >> configuring an already configured SEP similarly as in avdtp_setconf_cmd.
> >> ---
> >>
> >> Notes:
> >>     E.g. trying to set the same codec for two simultaneously connected
> >>     devices for the same adapter in Pulseaudio, causes the A2DP
> >>     connection of the first device stop working, as its transport
> >>     acquires start failing with EPERM. Disconnecting the first device
> >>     also breaks the second device connection.
> >>     This patch fixes it so that only the invalid SetConfiguration fails.
> >>
> >>  profiles/audio/a2dp.c  | 5 +++++
> >>  profiles/audio/avdtp.c | 3 +++
> >>  2 files changed, 8 insertions(+)
> >>
> >> diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> >> index 6f5b13711..f3e2cdd9e 100644
> >> --- a/profiles/audio/a2dp.c
> >> +++ b/profiles/audio/a2dp.c
> >> @@ -1843,6 +1843,11 @@ static int a2dp_reconfig(struct a2dp_channel *chan, const char *sender,
> >>         GSList *l;
> >>         int err;
> >>
> >> +       /* Check SEP not used by a different session */
> >> +       if (lsep->stream && chan->session &&
> >> +           !avdtp_has_stream(chan->session, lsep->stream))
> >> +               return -EBUSY;
> >> +
> >>         setup = a2dp_setup_get(chan->session);
> >>         if (!setup)
> >>                 return -ENOMEM;
> >> diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
> >> index da4114e0f..bc7afad81 100644
> >> --- a/profiles/audio/avdtp.c
> >> +++ b/profiles/audio/avdtp.c
> >> @@ -3523,6 +3523,9 @@ int avdtp_set_configuration(struct avdtp *session,
> >>         if (!(lsep && rsep))
> >>                 return -EINVAL;
> >>
> >> +       if (lsep->stream)
> >> +               return -EBUSY;
> >> +
> >>         DBG("%p: int_seid=%u, acp_seid=%u", session,
> >>                         lsep->info.seid, rsep->seid);
> >>
> >> --
> >> 2.36.1
> >>
> >
> >



-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 1/2] a2dp: disallow multiple SetConfiguration to same local SEP

[patchwork-bot+bluetooth]

Hello:

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

On Sun,  5 Jun 2022 15:29:26 +0300 you wrote:
> Using the remote SEP SetConfiguration DBus API, it's possible to make
> multiple remote endpoints use the same local SEP, if they are endpoints
> from different connected devices. This is invalid: successful
> configuration shall prevent a different device configuring the same SEP
> (AVDTP v1.3 Sec. 5.3).  Moreover, this breaks the assumption in the
> AVDTP code that each SEP has at most a single stream, and causes
> misbehavior later on (subsequent transport acquires fail with EPERM).
> 
> [...]

Here is the summary with links:
  - [BlueZ,1/2] a2dp: disallow multiple SetConfiguration to same local SEP
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=5f9d9a9a0b38
  - [BlueZ,2/2] a2dp: error return paths in a2dp_reconfig must free allocated setup
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=9c288dd23a3b

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH BlueZ 1/2] a2dp: disallow multiple SetConfiguration to same local SEP

[Luiz Augusto von Dentz]

Hi Pauli,

On Mon, Jun 13, 2022 at 12:47 PM <patchwork-bot+bluetooth@kernel.org> wrote:
>
> Hello:
>
> This series was applied to bluetooth/bluez.git (master)
> by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:
>
> On Sun,  5 Jun 2022 15:29:26 +0300 you wrote:
> > Using the remote SEP SetConfiguration DBus API, it's possible to make
> > multiple remote endpoints use the same local SEP, if they are endpoints
> > from different connected devices. This is invalid: successful
> > configuration shall prevent a different device configuring the same SEP
> > (AVDTP v1.3 Sec. 5.3).  Moreover, this breaks the assumption in the
> > AVDTP code that each SEP has at most a single stream, and causes
> > misbehavior later on (subsequent transport acquires fail with EPERM).
> >
> > [...]
>
> Here is the summary with links:
>   - [BlueZ,1/2] a2dp: disallow multiple SetConfiguration to same local SEP
>     https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=5f9d9a9a0b38
>   - [BlueZ,2/2] a2dp: error return paths in a2dp_reconfig must free allocated setup
>     https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=9c288dd23a3b
>
> You are awesome, thank you!
> --
> Deet-doot-dot, I am a bot.
> https://korg.docs.kernel.org/patchwork/pwbot.html
>

Ive ended applying since I'm not sure you would be working on
supporting reusing the same endpoint properly and leaving the code as
is not working properly.



-- 
Luiz Augusto von Dentz