| Message ID | 20250128174327.1477546-1-quic_amisjain@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] obex: Add supported features tag in MAP Client Connect Request | expand |
Hi Luiz, > -----Original Message----- > From: Luiz Augusto von Dentz <luiz.dentz@gmail.com> > Sent: Tuesday, January 28, 2025 11:20 PM > To: Amisha Jain (QUIC) <quic_amisjain@quicinc.com> > Cc: linux-bluetooth@vger.kernel.org; Mohammed Sameer Mulla (QUIC) > <quic_mohamull@quicinc.com>; Harish Bandi (QUIC) > <quic_hbandi@quicinc.com>; Anubhav Gupta (QUIC) > <quic_anubhavg@quicinc.com> > 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@quicinc.com> > 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 - 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 > > --- > > 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. > > + > > + 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
Hi Amisha, On Thu, Jan 30, 2025 at 2:38 PM Amisha Jain (QUIC) <quic_amisjain@quicinc.com> wrote: > > Hi Luiz, > > > -----Original Message----- > > From: Luiz Augusto von Dentz <luiz.dentz@gmail.com> > > Sent: Tuesday, January 28, 2025 11:20 PM > > To: Amisha Jain (QUIC) <quic_amisjain@quicinc.com> > > Cc: linux-bluetooth@vger.kernel.org; Mohammed Sameer Mulla (QUIC) > > <quic_mohamull@quicinc.com>; Harish Bandi (QUIC) > > <quic_hbandi@quicinc.com>; Anubhav Gupta (QUIC) > > <quic_anubhavg@quicinc.com> > > 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@quicinc.com> > > 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
Hi Luiz, > -----Original Message----- > From: Luiz Augusto von Dentz <luiz.dentz@gmail.com> > Sent: Friday, January 31, 2025 3:07 AM > To: Amisha Jain (QUIC) <quic_amisjain@quicinc.com> > Cc: linux-bluetooth@vger.kernel.org; Mohammed Sameer Mulla (QUIC) > <quic_mohamull@quicinc.com>; Harish Bandi (QUIC) > <quic_hbandi@quicinc.com>; Anubhav Gupta (QUIC) > <quic_anubhavg@quicinc.com> > Subject: Re: [PATCH v1] obex: Add supported features tag in MAP Client > Connect Request > > Hi Amisha, > > On Thu, Jan 30, 2025 at 2:38 PM Amisha Jain (QUIC) > <quic_amisjain@quicinc.com> wrote: > > > > Hi Luiz, > > > > > -----Original Message----- > > > From: Luiz Augusto von Dentz <luiz.dentz@gmail.com> > > > Sent: Tuesday, January 28, 2025 11:20 PM > > > To: Amisha Jain (QUIC) <quic_amisjain@quicinc.com> > > > Cc: linux-bluetooth@vger.kernel.org; Mohammed Sameer Mulla (QUIC) > > > <quic_mohamull@quicinc.com>; Harish Bandi (QUIC) > > > <quic_hbandi@quicinc.com>; Anubhav Gupta (QUIC) > > > <quic_anubhavg@quicinc.com> > > > 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@quicinc.com> > > > 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. > Sure, we will add the byte order check in next patch. For checking the size of attribute whether it is 32 bits or not can be tricky as the obc_session_get_attribute() will internally call the bluetooth_getattribute() function which only returns the attribute value in void* format. Even if we introduce new function like obc_session_get_attribute_le32 , it will also eventually call the bluetooth_getattribute(). One way to resolve this is to change the return type of bluetooth_getattribute() to 'struct sdp_data_t' which will have the 'unitSize' info. This function is common call for many profiles hence more changes will be involved. Other way to resolve this is to add new function like bluetooth_getattribute_32() which will internally check the data size and return it. Please suggest any other way to resolve this or if we can go ahead with one of the above approaches. > > > > > > + > > > > + 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 Thanks, Amisha
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); + + 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 };