Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220605122927.110627-1-pav@iki.fi>
In-Reply-To: <20220605122927.110627-1-pav@iki.fi>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 6 Jun 2022 22:33:46 -0700
Message-ID: <CABBYNZJ4f-wxZwPdYWzxQWHfG+x46HMiPZ=TmG7S74DGhPVW2A@mail.gmail.com>
Subject: Re: [PATCH BlueZ 1/2] a2dp: disallow multiple SetConfiguration to
 same local SEP
To:     Pauli Virtanen <pav@iki.fi>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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