Hi Amisha,
On Thu, Jan 30, 2025 at 2:38 PM Amisha Jain (QUIC)
<quic_amisjain@xxxxxxxxxxx> wrote:
>
> Hi Luiz,
>
> > -----Original Message-----
> > From: Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
> > Sent: Tuesday, January 28, 2025 11:20 PM
> > To: Amisha Jain (QUIC) <quic_amisjain@xxxxxxxxxxx>
> > Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Mohammed Sameer Mulla (QUIC)
> > <quic_mohamull@xxxxxxxxxxx>; Harish Bandi (QUIC)
> > <quic_hbandi@xxxxxxxxxxx>; Anubhav Gupta (QUIC)
> > <quic_anubhavg@xxxxxxxxxxx>
> > Subject: Re: [PATCH v1] obex: Add supported features tag in MAP Client
> > Connect Request
> >
> > Hi Amisha,
> >
> > On Tue, Jan 28, 2025 at 12:43 PM Amisha Jain <quic_amisjain@xxxxxxxxxxx>
> > wrote:
> > >
> > > This change is required for below PTS testcase -
> > >
> > > 1. MAP/MCE/MFB/BV-06-C
> > > Verify that the MCE sends its MapSupportedFeatures in the OBEX Connect
> > > request if the MSE declares support for the feature
> > > MapSupportedFeatures in Connect Request in its SDP record.
> > >
> > > If Server's SDP record contains the field 'MapSupportedFeatures in
> > > Connect Request' as supported then include the supported features tag
> > > in obex connect request.
> >
> > Can you include the btmon output with and without?
> >
> btmon output does not captures obex packet so adding snoop packet -
Hmm, seems you are right, we only had it for hcidump not btmon.
> Obex Connect request with this change -
> OBEX
> Role: Central
> Address: 0
> Packet Status: Final Packet
> Opcode: Connect
> Length: 35
> OBEX Version Number: 0x10
> flags
> Maximum Packet Length: 1800
> Target
> Header Encoding: Byte Sequence
> Header ID: Target
> Length: 19
> Target: MAS
> Application Parameters
> Header Encoding: Byte Sequence
> Header ID: Application Parameters
> Length: 9
> Parameter
> Tag: Map Supported Features
> Length: 4
> Value
> : Messages-Listing Format Version 1.1
> : Extended Event Report 1.1
> : Instance Information Feature
> : Delete Feature
> : Uploading Feature
> : Browsing Feature
> : Notification Feature
> : Notification Registration Feature
>
> Obex Connect request without this change -
> OBEX
> Role: Central
> Address: 0
> Packet Status: Final Packet
> Opcode: Connect
> Length: 26
> OBEX Version Number: 0x10
> flags
> Maximum Packet Length: 1800
> Target
> Header Encoding: Byte Sequence
> Header ID: Target
> Length: 19
> Target: MAS
Please include this info as part of the patch description.
> > > ---
> > > obexd/client/map.c | 19 +++++++++++++++++++
> > > 1 file changed, 19 insertions(+)
> > >
> > > diff --git a/obexd/client/map.c b/obexd/client/map.c index
> > > b8820335b..2fd0f74ec 100644
> > > --- a/obexd/client/map.c
> > > +++ b/obexd/client/map.c
> > > @@ -39,6 +39,7 @@
> > > #define OBEX_MAS_UUID \
> > >
> > "\xBB\x58\x2B\x40\x42\x0C\x11\xDB\xB0\xDE\x08\x00\x20\x0C\x9A\x66"
> > > #define OBEX_MAS_UUID_LEN 16
> > > +#define SUPPORTED_FEATURES_TAG 0x29
> > >
> > > #define MAP_INTERFACE "org.bluez.obex.MessageAccess1"
> > > #define MAP_MSG_INTERFACE "org.bluez.obex.Message1"
> > > @@ -2179,6 +2180,23 @@ static void parse_service_record(struct
> > map_data *map)
> > > map->supported_features = 0x0000001f; }
> > >
> > > +static void *map_supported_features(struct obc_session *session) {
> > > + const void *data;
> > > +
> > > + /* Supported Feature Bits */
> > > + data = obc_session_get_attribute(session,
> > > + SDP_ATTR_MAP_SUPPORTED_FEATURES);
> > > + if (!data)
> > > + return NULL;
> > > +
> > > + if(*(uint32_t *)data & 0x00080000)
> > > + return g_obex_apparam_set_uint32(NULL,
> > SUPPORTED_FEATURES_TAG,
> > > + 0x0000027f);
> >
> > Don't think it is safe to check the data like above, we don't know if field
> > returned is really 32 bits, perhaps it would be a good idea to introduce
> > something like obc_session_get_attribute_le32 that would ensure the value is
> > really 32 bits and also check its little/big endian in the process.
> >
>
> As per the BT Spec, 32 bits field is reserved for 'MapSupportedFeatures' attribute in SDP record. So, it will be always 32 bits. Each bit corresponds to each feature. If any feature is not supported then that bit will be zero.
First 32 bits is not a proper format without us knowing in what byte
order it is, second we need to actually check the attribute is
correctly formatted and properly do it matches the expected size, now
perhaps you are saying that is our own SDP record we are talking
about, but that doesn't change the fact that there should be checks to
make sure we don't access invalid memory if something goes wrong.
>
> > > +
> > > + return NULL;
> > > +}
> > > +
> > > static int map_probe(struct obc_session *session) {
> > > struct map_data *map;
> > > @@ -2224,6 +2242,7 @@ static struct obc_driver map = {
> > > .uuid = MAS_UUID,
> > > .target = OBEX_MAS_UUID,
> > > .target_len = OBEX_MAS_UUID_LEN,
> > > + .supported_features = map_supported_features,
> > > .probe = map_probe,
> > > .remove = map_remove
> > > };
> > > --
> > > 2.34.1
> > >
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
> Thanks,
> Amisha
--
Luiz Augusto von Dentz
