Hi,
On Wed, Jan 3, 2024 at 10:38 AM Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
>
> Hi,
>
> On Wed, Jan 3, 2024 at 9:40 AM Xiao Yao <xiaokeqinhealth@xxxxxxx> wrote:
> >
> > From: Xiao Yao <xiaoyao@xxxxxxxxxxxxxx>
> >
> > 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 corrupted
> > messages. Those messages are discarded and no signaling message is returned
> > to the sender if no error code is applicable. Possible corrupted messages
> > are:
> >
> > * Response messages where the transaction label cannot match a previous
> > 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 nosp 0
> > AVDTP: Get All Capabilities (0x0c) Resp Accept (0x02) type 0 label 6 nosp 0
> > AVDTP: Get All Capabilities (0x0c) Command (0x00) type 0x00 label 7 nosp 0
> > AVDTP: Get All Capabilities (0x0c) Resp Accept (0x02) type 0 label 7 nosp 0
> >
> > < AVDTP: Set Configuration (0x03) Command (0x00) type 0x00 label 8 nosp 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 command.
> > < 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 nosp 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_ACCEPT,
> > 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 nosp 0
> > AVDTP: Get All Capabilities (0x0c) Command (0x00) type 0x00 label 2 nosp 0
> > AVDTP: Get All Capabilities (0x0c) Resp Accept (0x02) type 0 label 2 nosp 0
> > ... ...
> >
> > Therefore, a async_transaction that requires asynchronous return is
> > recorded to prevent it from being incorrectly modified.
Btw, we can probably come up with a test for this on unit/test-avdtp
to ensure we cover it.
> >
> > Signed-off-by: Xiao Yao <xiaoyao@xxxxxxxxxxxxxx>
> > ---
> > 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?
>
> > 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 != NULL) {
> > rej.error = AVDTP_UNSUPPORTED_CONFIGURATION;
> > rej.category = err->err.error_code;
> > - avdtp_send(session, session->in.transaction,
> > - AVDTP_MSG_TYPE_REJECT, AVDTP_SET_CONFIGURATION,
> > - &rej, sizeof(rej));
> > + avdtp_send(session, session->in.async_transaction,
> > + AVDTP_MSG_TYPE_REJECT, AVDTP_SET_CONFIGURATION,
> > + &rej, sizeof(rej));
> > stream_free(stream);
> > return;
> > }
> >
> > - if (!avdtp_send(session, session->in.transaction, AVDTP_MSG_TYPE_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 = 0x0103;
> >
> > if (sep->ind && sep->ind->set_configuration) {
> > + /* The setconfig configuration stage is asynchronous;
> > + * high CPU load or other factors can delay dbus message
> > + * responses, potentially altering the transaction value.
> > + * It's essential to record the received transaction value
> > + * for use in the final accept command.
> > + */
> > + session->in.async_transaction = transaction;
> > if (!sep->ind->set_configuration(session, sep, stream,
> > stream->caps,
> > setconf_cb,
> >
> > base-commit: 7ad5669402c9acff8e4cc808edc12a41df36654e
> > --
> > 2.34.1
> >
> >
>
>
> --
> Luiz Augusto von Dentz
--
Luiz Augusto von Dentz
