Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-868-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240103143904.77146-1-xiaokeqinhealth@126.com>
 <CABBYNZLo9CVZsjvzjKsGKFyhOkrWoyWYL8bQoSfvTDf=PAQ-iw@mail.gmail.com> <5df7515e-5785-400c-8e29-42fc88cef0c6@126.com>
In-Reply-To: <5df7515e-5785-400c-8e29-42fc88cef0c6@126.com>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Wed, 3 Jan 2024 13:49:30 -0500
Message-ID: <CABBYNZ+T9PJDWgdyfKdTXB1gpuLdvrb3BycUW-pwCLFSLvmGhA@mail.gmail.com>
Subject: Re: [PATCH BlueZ v3 1/2] avdtp: fix incorrect transaction label in
 setconf phase
To: Yao Xiao <xiaokeqinhealth@126.com>
Cc: linux-bluetooth@vger.kernel.org, Xiao Yao <xiaoyao@rock-chips.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Wed, Jan 3, 2024 at 1:41=E2=80=AFPM Yao Xiao <xiaokeqinhealth@126.com> w=
rote:
>
> Hi,
> On 2024/1/3 23:38, Luiz Augusto von Dentz wrote:
> > Hi,
> >
> > On Wed, Jan 3, 2024 at 9:40=E2=80=AFAM Xiao Yao <xiaokeqinhealth@126.co=
m> wrote:
> >> From: Xiao Yao <xiaoyao@rock-chips.com>
> >>
> >> BLUETOOTH SPECIFICATION Page 61 of 140
> >> Audio/Video Distribution Transport Protocol Specification (V13)
> >> 8.4.6 Message integrity verification at receiver side
> >>
> >> - The receiver of an AVDTP signaling message shall not interpret corru=
pted
> >> messages. Those messages are discarded and no signaling message is ret=
urned
> >> to the sender if no error code is applicable. Possible corrupted messa=
ges
> >> are:
> >>
> >>    * Response messages where the transaction label cannot match a prev=
ious
> >>      command sent to the remote device
> >>
> >> Consider the following scenario:
> >> btmon log:
> >> ... ...
> >> AVDTP: Discover (0x01) Command (0x00) type 0x00 label 5 nosp 0
> >> AVDTP: Discover (0x01) Response Accept (0x02) type 0x00 label 5 nosp 0
> >> AVDTP: Get All Capabilities (0x0c) Command (0x00) type 0x00 label 6 no=
sp 0
> >> AVDTP: Get All Capabilities (0x0c) Resp Accept (0x02) type 0 label 6 n=
osp 0
> >> AVDTP: Get All Capabilities (0x0c) Command (0x00) type 0x00 label 7 no=
sp 0
> >> AVDTP: Get All Capabilities (0x0c) Resp Accept (0x02) type 0 label 7 n=
osp 0
> >>
> >> < AVDTP: Set Configuration (0x03) Command (0x00) type 0x00 label 8 nos=
p 0
> >> //Currently, a 'set configuration' message has been received from the
> >> //sender, which contains a transaction label valued at 8. This message
> >> //was then relayed to A2DP backend(PulseAudio/PipeWire) using the dbus
> >> //interface.
> >>    set_configuration()(media.c)
> >>      dbus_message_new_method_call(..., "SetConfiguration", ...);
> >>      g_dbus_send_message_with_reply(btd_get_dbus_connection(), ...);
> >>      dbus_pending_call_set_notify(request->call, endpoint_reply, ...);
> >>      ...
> >>
> >>> AVDTP: Discover (0x01) Command (0x00) type 0x00 label 0 nosp 0
> >> //At this time, the A2DP reverse discovery issued an A2DP discover com=
mand.
> >> < AVDTP: Discover (0x01) Response Accept (0x02) type 0x00 label 0 nosp=
 0
> >> //After receiving the discover reply, the session->in.transaction is
> >> //changed to 0
> >>
> >>> AVDTP: Set Configuration (0x03) Resp Accept (0x02) type 0 label 0 nos=
p 0
> >> //The audio backend reply the dbus message
> >>    endpoint_reply (media.c)
> >>      setconf_cb (avdtp.c)
> >>        //Here avdtp_send sends an incorrect transaction value, causing
> >>        //the sender to discard the message. (The correct transaction
> >>        //value is 8)
> >>        avdtp_send(session, session->in.transaction, AVDTP_MSG_TYPE_ACC=
EPT,
> >>                   AVDTP_SET_CONFIGURATION, NULL, 0)
> >>
> >> AVDTP: Delay Report (0x0d) Command (0x00) type 0x00 label 1 nosp 0
> >> AVDTP: Delay Report (0x0d) Response Accept (0x02) type 0x00 label 1 no=
sp 0
> >> AVDTP: Get All Capabilities (0x0c) Command (0x00) type 0x00 label 2 no=
sp 0
> >> AVDTP: Get All Capabilities (0x0c) Resp Accept (0x02) type 0 label 2 n=
osp 0
> >> ... ...
> >>
> >> Therefore, a async_transaction that requires asynchronous return is
> >> recorded to prevent it from being incorrectly modified.
> >>
> >> Signed-off-by: Xiao Yao <xiaoyao@rock-chips.com>
> >> ---
> >> v1 -> v2: Fixed "session->in.transaction" logic err.
> >> v2 -> v3: Fixed some compile warnings
> >> ---
> >>   profiles/audio/avdtp.c | 19 ++++++++++++++-----
> >>   1 file changed, 14 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
> >> index 10ef380d4..386c7f67c 100644
> >> --- a/profiles/audio/avdtp.c
> >> +++ b/profiles/audio/avdtp.c
> >> @@ -286,6 +286,7 @@ struct in_buf {
> >>          gboolean active;
> >>          int no_of_packets;
> >>          uint8_t transaction;
> >> +       uint8_t async_transaction;
> > Didn't I already explain it already that we are not supposed to have 2
> > outstanding transactions?
> Apologies for the misunderstanding earlier. When you mentioned
> 'outstanding transactions',
> were you referring to a complete configuration process, rather than a
> single command?
> Can I understand it this way: An a2dp discovery command should not be
> sent before responding
> to the AVDTP_MSG_TYPE_ACCEPT message? Is it only after sending the
> AVDTP_MSG_TYPE_ACCEPT
> response to the remote device that I can proceed with the a2dp discovery?

You can have a single outstanding transaction in each direction, but
not 2 like in.transaction + in.async_transaction since that means we
received a second request while 1 is still outstanding.

> >
> >>          uint8_t message_type;
> >>          uint8_t signal_id;
> >>          uint8_t buf[1024];
> >> @@ -1462,15 +1463,16 @@ static void setconf_cb(struct avdtp *session, =
struct avdtp_stream *stream,
> >>          if (err !=3D NULL) {
> >>                  rej.error =3D AVDTP_UNSUPPORTED_CONFIGURATION;
> >>                  rej.category =3D err->err.error_code;
> >> -               avdtp_send(session, session->in.transaction,
> >> -                               AVDTP_MSG_TYPE_REJECT, AVDTP_SET_CONFI=
GURATION,
> >> -                               &rej, sizeof(rej));
> >> +               avdtp_send(session, session->in.async_transaction,
> >> +                          AVDTP_MSG_TYPE_REJECT, AVDTP_SET_CONFIGURAT=
ION,
> >> +                          &rej, sizeof(rej));
> >>                  stream_free(stream);
> >>                  return;
> >>          }
> >>
> >> -       if (!avdtp_send(session, session->in.transaction, AVDTP_MSG_TY=
PE_ACCEPT,
> >> -                                       AVDTP_SET_CONFIGURATION, NULL,=
 0)) {
> >> +       if (!avdtp_send(session, session->in.async_transaction,
> >> +                       AVDTP_MSG_TYPE_ACCEPT,
> >> +                       AVDTP_SET_CONFIGURATION, NULL, 0)) {
> >>                  stream_free(stream);
> >>                  return;
> >>          }
> >> @@ -1569,6 +1571,13 @@ static gboolean avdtp_setconf_cmd(struct avdtp =
*session, uint8_t transaction,
> >>                  session->version =3D 0x0103;
> >>
> >>          if (sep->ind && sep->ind->set_configuration) {
> >> +               /* The setconfig configuration stage is asynchronous;
> >> +                * high CPU load or other factors can delay dbus messa=
ge
> >> +                * responses, potentially altering the transaction val=
ue.
> >> +                * It's essential to record the received transaction v=
alue
> >> +                * for use in the final accept command.
> >> +                */
> >> +               session->in.async_transaction =3D transaction;
> >>                  if (!sep->ind->set_configuration(session, sep, stream=
,
> >>                                                          stream->caps,
> >>                                                          setconf_cb,
> >>
> >> base-commit: 7ad5669402c9acff8e4cc808edc12a41df36654e
> >> --
> >> 2.34.1
> >>
> >>
> >
>


--=20
Luiz Augusto von Dentz