[BlueZ,v3,1/3] audio/avrcp: Guard SetAbsoluteVolume without target behind config value

Commit 179ccb936 ("avrcp: Set volume if volume changed event is
registered") introduced a catch that allows SetAbsoluteVolume to be sent
to a remote device that does _not_ implement the AVRCP TG profile.  This
is strange as the TG role is required to be able to send commands to the
peer, but the commit must have been applied to the tree for a reason.

We discussed in [1] that workarounds for dubious peers and software
stacks should be guarded behind a config entry in main.conf, so this
starts out by introducing a new [AVRCP] category to to it that will
later be extended with other workarounds.

[1]: https://marc.info/?l=linux-bluetooth&m=163519566912788&w=2
---
 profiles/audio/avrcp.c | 12 +++++++++---
 src/btd.h              |  5 +++++
 src/main.c             | 13 +++++++++++++
 src/main.conf          |  6 ++++++
 4 files changed, 33 insertions(+), 3 deletions(-)

Hi Marijn,

On Fri, Mar 10, 2023 at 4:39 PM Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> Commit 179ccb936 ("avrcp: Set volume if volume changed event is
> registered") introduced a catch that allows SetAbsoluteVolume to be sent
> to a remote device that does _not_ implement the AVRCP TG profile.  This
> is strange as the TG role is required to be able to send commands to the
> peer, but the commit must have been applied to the tree for a reason.
>
> We discussed in [1] that workarounds for dubious peers and software
> stacks should be guarded behind a config entry in main.conf, so this
> starts out by introducing a new [AVRCP] category to to it that will
> later be extended with other workarounds.
>
> [1]: https://marc.info/?l=linux-bluetooth&m=163519566912788&w=2
> ---
>  profiles/audio/avrcp.c | 12 +++++++++---
>  src/btd.h              |  5 +++++
>  src/main.c             | 13 +++++++++++++
>  src/main.conf          |  6 ++++++
>  4 files changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> index 80f34c7a7..5e6322916 100644
> --- a/profiles/audio/avrcp.c
> +++ b/profiles/audio/avrcp.c
> @@ -48,6 +48,7 @@
>  #include "src/dbus-common.h"
>  #include "src/shared/timeout.h"
>  #include "src/shared/util.h"
> +#include "src/btd.h"
>
>  #include "avctp.h"
>  #include "avrcp.h"
> @@ -4577,9 +4578,14 @@ int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify)
>                                                                 &volume);
>         }
>
> -       if (!session->controller && !avrcp_event_registered(session,
> -                                       AVRCP_EVENT_VOLUME_CHANGED))
> -               return -ENOTSUP;
> +       if (btd_opts.avrcp.set_absolute_volume_without_target) {
> +               if (!session->controller && !avrcp_event_registered(session,
> +                                               AVRCP_EVENT_VOLUME_CHANGED))
> +                       return -ENOTSUP;
> +       } else {
> +               if (!session->controller || session->controller->version < 0x0104)
> +                       return -ENOTSUP;
> +       }
>
>         memset(buf, 0, sizeof(buf));
>
> diff --git a/src/btd.h b/src/btd.h
> index 42cffcde4..31c04a990 100644
> --- a/src/btd.h
> +++ b/src/btd.h
> @@ -97,6 +97,10 @@ struct btd_avdtp_opts {
>         uint8_t  stream_mode;
>  };
>
> +struct btd_avrcp_opts {
> +       gboolean set_absolute_volume_without_target;
> +};
> +
>  struct btd_advmon_opts {
>         uint8_t         rssi_sampling_period;
>  };
> @@ -136,6 +140,7 @@ struct btd_opts {
>         enum mps_mode_t mps;
>
>         struct btd_avdtp_opts avdtp;
> +       struct btd_avrcp_opts avrcp;
>
>         uint8_t         key_size;
>
> diff --git a/src/main.c b/src/main.c
> index 99d9c508f..92f74e381 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -152,6 +152,11 @@ static const char *avdtp_options[] = {
>         NULL
>  };
>
> +static const char *avrcp_options[] = {
> +       "SetAbsoluteVolumeWithoutTarget",
> +       NULL
> +};
> +
>  static const char *advmon_options[] = {
>         "RSSISamplingPeriod",
>         NULL
> @@ -167,6 +172,7 @@ static const struct group_table {
>         { "Policy",     policy_options },
>         { "GATT",       gatt_options },
>         { "AVDTP",      avdtp_options },
> +       { "AVRCP",      avrcp_options },
>         { "AdvMon",     advmon_options },
>         { }
>  };
> @@ -975,6 +981,13 @@ static void parse_config(GKeyFile *config)
>                 g_free(str);
>         }
>
> +       boolean = g_key_file_get_boolean(config, "AVRCP",
> +                                               "SetAbsoluteVolumeWithoutTarget", &err);
> +       if (err)
> +               g_clear_error(&err);
> +       else
> +               btd_opts.avrcp.set_absolute_volume_without_target = boolean;
> +
>         val = g_key_file_get_integer(config, "AdvMon", "RSSISamplingPeriod",
>                                                                         &err);
>         if (err) {
> diff --git a/src/main.conf b/src/main.conf
> index f187c9aaa..ca00ed03e 100644
> --- a/src/main.conf
> +++ b/src/main.conf
> @@ -271,6 +271,12 @@
>  # streaming: Use L2CAP Streaming Mode
>  #StreamMode = basic
>
> +[AVRCP]
> +# Allow SetAbsoluteVolume calls to a peer device that
> +# does not advertise the AVRCP remote control target
> +# profile.
> +#SetAbsoluteVolumeWithoutTarget = false

Let's do just VolumeWithoutTarget and we should probably mention that
it would ignore the version as well.

> +
>  [Policy]
>  #
>  # The ReconnectUUIDs defines the set of remote services that should try
> --
> 2.39.2
>

Hi Luiz,

I'm still clueless why this reply didn't reach my inbox.  I thought it
was going into ignore-land again (sorry) but randomly found your replies
on the BlueZ mailing list.  The bot message didn't make it through
either, but it did notify me about "audio/transport: Propagate errors
from avrcp_set_volume to DBus" being applied.

On 2023-03-14 16:16:55, Luiz Augusto von Dentz wrote:
> Hi Marijn,
> 
> On Fri, Mar 10, 2023 at 4:39 PM Marijn Suijten
> <marijn.suijten@somainline.org> wrote:
> >
> > Commit 179ccb936 ("avrcp: Set volume if volume changed event is
> > registered") introduced a catch that allows SetAbsoluteVolume to be sent
> > to a remote device that does _not_ implement the AVRCP TG profile.  This
> > is strange as the TG role is required to be able to send commands to the
> > peer, but the commit must have been applied to the tree for a reason.
> >
> > We discussed in [1] that workarounds for dubious peers and software
> > stacks should be guarded behind a config entry in main.conf, so this
> > starts out by introducing a new [AVRCP] category to to it that will
> > later be extended with other workarounds.
> >
> > [1]: https://marc.info/?l=linux-bluetooth&m=163519566912788&w=2
> > ---
> >  profiles/audio/avrcp.c | 12 +++++++++---
> >  src/btd.h              |  5 +++++
> >  src/main.c             | 13 +++++++++++++
> >  src/main.conf          |  6 ++++++
> >  4 files changed, 33 insertions(+), 3 deletions(-)
> >
> > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> > index 80f34c7a7..5e6322916 100644
> > --- a/profiles/audio/avrcp.c
> > +++ b/profiles/audio/avrcp.c
> > @@ -48,6 +48,7 @@
> >  #include "src/dbus-common.h"
> >  #include "src/shared/timeout.h"
> >  #include "src/shared/util.h"
> > +#include "src/btd.h"
> >
> >  #include "avctp.h"
> >  #include "avrcp.h"
> > @@ -4577,9 +4578,14 @@ int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify)
> >                                                                 &volume);
> >         }
> >
> > -       if (!session->controller && !avrcp_event_registered(session,
> > -                                       AVRCP_EVENT_VOLUME_CHANGED))
> > -               return -ENOTSUP;
> > +       if (btd_opts.avrcp.set_absolute_volume_without_target) {
> > +               if (!session->controller && !avrcp_event_registered(session,
> > +                                               AVRCP_EVENT_VOLUME_CHANGED))
> > +                       return -ENOTSUP;
> > +       } else {
> > +               if (!session->controller || session->controller->version < 0x0104)
> > +                       return -ENOTSUP;
> > +       }
> >
> >         memset(buf, 0, sizeof(buf));
> >
> > diff --git a/src/btd.h b/src/btd.h
> > index 42cffcde4..31c04a990 100644
> > --- a/src/btd.h
> > +++ b/src/btd.h
> > @@ -97,6 +97,10 @@ struct btd_avdtp_opts {
> >         uint8_t  stream_mode;
> >  };
> >
> > +struct btd_avrcp_opts {
> > +       gboolean set_absolute_volume_without_target;
> > +};
> > +
> >  struct btd_advmon_opts {
> >         uint8_t         rssi_sampling_period;
> >  };
> > @@ -136,6 +140,7 @@ struct btd_opts {
> >         enum mps_mode_t mps;
> >
> >         struct btd_avdtp_opts avdtp;
> > +       struct btd_avrcp_opts avrcp;
> >
> >         uint8_t         key_size;
> >
> > diff --git a/src/main.c b/src/main.c
> > index 99d9c508f..92f74e381 100644
> > --- a/src/main.c
> > +++ b/src/main.c
> > @@ -152,6 +152,11 @@ static const char *avdtp_options[] = {
> >         NULL
> >  };
> >
> > +static const char *avrcp_options[] = {
> > +       "SetAbsoluteVolumeWithoutTarget",
> > +       NULL
> > +};
> > +
> >  static const char *advmon_options[] = {
> >         "RSSISamplingPeriod",
> >         NULL
> > @@ -167,6 +172,7 @@ static const struct group_table {
> >         { "Policy",     policy_options },
> >         { "GATT",       gatt_options },
> >         { "AVDTP",      avdtp_options },
> > +       { "AVRCP",      avrcp_options },
> >         { "AdvMon",     advmon_options },
> >         { }
> >  };
> > @@ -975,6 +981,13 @@ static void parse_config(GKeyFile *config)
> >                 g_free(str);
> >         }
> >
> > +       boolean = g_key_file_get_boolean(config, "AVRCP",
> > +                                               "SetAbsoluteVolumeWithoutTarget", &err);
> > +       if (err)
> > +               g_clear_error(&err);
> > +       else
> > +               btd_opts.avrcp.set_absolute_volume_without_target = boolean;
> > +
> >         val = g_key_file_get_integer(config, "AdvMon", "RSSISamplingPeriod",
> >                                                                         &err);
> >         if (err) {
> > diff --git a/src/main.conf b/src/main.conf
> > index f187c9aaa..ca00ed03e 100644
> > --- a/src/main.conf
> > +++ b/src/main.conf
> > @@ -271,6 +271,12 @@
> >  # streaming: Use L2CAP Streaming Mode
> >  #StreamMode = basic
> >
> > +[AVRCP]
> > +# Allow SetAbsoluteVolume calls to a peer device that
> > +# does not advertise the AVRCP remote control target
> > +# profile.
> > +#SetAbsoluteVolumeWithoutTarget = false
> 
> Let's do just VolumeWithoutTarget and we should probably mention that
> it would ignore the version as well.

Sure, sounds good.  We can also invert the condition to validate the
version in the event that ->controller is non-NULL, even if
VolumeWithoutTarget is set.

- Marijn

> 
> > +
> >  [Policy]
> >  #
> >  # The ReconnectUUIDs defines the set of remote services that should try
> > --
> > 2.39.2
> >
> 
> 
> -- 
> Luiz Augusto von Dentz

Hi Marijn,

On Tue, May 30, 2023 at 3:18 PM Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> Hi Luiz,
>
> I'm still clueless why this reply didn't reach my inbox.  I thought it
> was going into ignore-land again (sorry) but randomly found your replies
> on the BlueZ mailing list.  The bot message didn't make it through
> either, but it did notify me about "audio/transport: Propagate errors
> from avrcp_set_volume to DBus" being applied.

Now that you mentioned it, I did receive:

The response from the remote server was:

550 5.7.1 Blocked due to message content - please contact postmaster@seeweb.it

So perhaps that is the reason you didn't get any responses for a while.


> On 2023-03-14 16:16:55, Luiz Augusto von Dentz wrote:
> > Hi Marijn,
> >
> > On Fri, Mar 10, 2023 at 4:39 PM Marijn Suijten
> > <marijn.suijten@somainline.org> wrote:
> > >
> > > Commit 179ccb936 ("avrcp: Set volume if volume changed event is
> > > registered") introduced a catch that allows SetAbsoluteVolume to be sent
> > > to a remote device that does _not_ implement the AVRCP TG profile.  This
> > > is strange as the TG role is required to be able to send commands to the
> > > peer, but the commit must have been applied to the tree for a reason.
> > >
> > > We discussed in [1] that workarounds for dubious peers and software
> > > stacks should be guarded behind a config entry in main.conf, so this
> > > starts out by introducing a new [AVRCP] category to to it that will
> > > later be extended with other workarounds.
> > >
> > > [1]: https://marc.info/?l=linux-bluetooth&m=163519566912788&w=2
> > > ---
> > >  profiles/audio/avrcp.c | 12 +++++++++---
> > >  src/btd.h              |  5 +++++
> > >  src/main.c             | 13 +++++++++++++
> > >  src/main.conf          |  6 ++++++
> > >  4 files changed, 33 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> > > index 80f34c7a7..5e6322916 100644
> > > --- a/profiles/audio/avrcp.c
> > > +++ b/profiles/audio/avrcp.c
> > > @@ -48,6 +48,7 @@
> > >  #include "src/dbus-common.h"
> > >  #include "src/shared/timeout.h"
> > >  #include "src/shared/util.h"
> > > +#include "src/btd.h"
> > >
> > >  #include "avctp.h"
> > >  #include "avrcp.h"
> > > @@ -4577,9 +4578,14 @@ int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify)
> > >                                                                 &volume);
> > >         }
> > >
> > > -       if (!session->controller && !avrcp_event_registered(session,
> > > -                                       AVRCP_EVENT_VOLUME_CHANGED))
> > > -               return -ENOTSUP;
> > > +       if (btd_opts.avrcp.set_absolute_volume_without_target) {
> > > +               if (!session->controller && !avrcp_event_registered(session,
> > > +                                               AVRCP_EVENT_VOLUME_CHANGED))
> > > +                       return -ENOTSUP;
> > > +       } else {
> > > +               if (!session->controller || session->controller->version < 0x0104)
> > > +                       return -ENOTSUP;
> > > +       }
> > >
> > >         memset(buf, 0, sizeof(buf));
> > >
> > > diff --git a/src/btd.h b/src/btd.h
> > > index 42cffcde4..31c04a990 100644
> > > --- a/src/btd.h
> > > +++ b/src/btd.h
> > > @@ -97,6 +97,10 @@ struct btd_avdtp_opts {
> > >         uint8_t  stream_mode;
> > >  };
> > >
> > > +struct btd_avrcp_opts {
> > > +       gboolean set_absolute_volume_without_target;
> > > +};
> > > +
> > >  struct btd_advmon_opts {
> > >         uint8_t         rssi_sampling_period;
> > >  };
> > > @@ -136,6 +140,7 @@ struct btd_opts {
> > >         enum mps_mode_t mps;
> > >
> > >         struct btd_avdtp_opts avdtp;
> > > +       struct btd_avrcp_opts avrcp;
> > >
> > >         uint8_t         key_size;
> > >
> > > diff --git a/src/main.c b/src/main.c
> > > index 99d9c508f..92f74e381 100644
> > > --- a/src/main.c
> > > +++ b/src/main.c
> > > @@ -152,6 +152,11 @@ static const char *avdtp_options[] = {
> > >         NULL
> > >  };
> > >
> > > +static const char *avrcp_options[] = {
> > > +       "SetAbsoluteVolumeWithoutTarget",
> > > +       NULL
> > > +};
> > > +
> > >  static const char *advmon_options[] = {
> > >         "RSSISamplingPeriod",
> > >         NULL
> > > @@ -167,6 +172,7 @@ static const struct group_table {
> > >         { "Policy",     policy_options },
> > >         { "GATT",       gatt_options },
> > >         { "AVDTP",      avdtp_options },
> > > +       { "AVRCP",      avrcp_options },
> > >         { "AdvMon",     advmon_options },
> > >         { }
> > >  };
> > > @@ -975,6 +981,13 @@ static void parse_config(GKeyFile *config)
> > >                 g_free(str);
> > >         }
> > >
> > > +       boolean = g_key_file_get_boolean(config, "AVRCP",
> > > +                                               "SetAbsoluteVolumeWithoutTarget", &err);
> > > +       if (err)
> > > +               g_clear_error(&err);
> > > +       else
> > > +               btd_opts.avrcp.set_absolute_volume_without_target = boolean;
> > > +
> > >         val = g_key_file_get_integer(config, "AdvMon", "RSSISamplingPeriod",
> > >                                                                         &err);
> > >         if (err) {
> > > diff --git a/src/main.conf b/src/main.conf
> > > index f187c9aaa..ca00ed03e 100644
> > > --- a/src/main.conf
> > > +++ b/src/main.conf
> > > @@ -271,6 +271,12 @@
> > >  # streaming: Use L2CAP Streaming Mode
> > >  #StreamMode = basic
> > >
> > > +[AVRCP]
> > > +# Allow SetAbsoluteVolume calls to a peer device that
> > > +# does not advertise the AVRCP remote control target
> > > +# profile.
> > > +#SetAbsoluteVolumeWithoutTarget = false
> >
> > Let's do just VolumeWithoutTarget and we should probably mention that
> > it would ignore the version as well.
>
> Sure, sounds good.  We can also invert the condition to validate the
> version in the event that ->controller is non-NULL, even if
> VolumeWithoutTarget is set.
>
> - Marijn
>
> >
> > > +
> > >  [Policy]
> > >  #
> > >  # The ReconnectUUIDs defines the set of remote services that should try
> > > --
> > > 2.39.2
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz