[PATCH BlueZ 0/4] Fix an allocation oversight in SDP parsing

[PATCH BlueZ 0/4] Fix an allocation oversight in SDP parsing

[Jonas Dreßler]

There's fairly old oversight in the SDP parsing code where it was forgotten to
add a NULL termination byte to strings that are later handled using strlen().

This series fixes that oversight, with a few commits to better follow best
practices on top.

Found by running with address sanitizer.

Jonas Dreßler (4):
  lib/sdp: Allocate strings in sdp_data_t with NULL termination
  lib/sdp: Don't assume uint8_t has size 1
  lib/sdp: Use correct string length in sdp_copy_seq()
  lib/sdp: Pass size_t to sdp_get_string_attr()

 lib/sdp.c     | 15 ++++++++-------
 lib/sdp_lib.h | 14 +++++++-------
 2 files changed, 15 insertions(+), 14 deletions(-)

-- 
2.41.0

[PATCH BlueZ 1/4] lib/sdp: Allocate strings in sdp_data_t with NULL termination

[Jonas Dreßler]

In extract_str() we create sdp_data_t with strings and allocate
sdp_data_t->val.str an extra 0-byte as NULL termination. In
sdp_data_alloc_with_length() we're missing this, and strlen() in
sdp_get_string_attr() ends up overrunning the sdpdata->val.str buffer
looking for the NULL termination.

Allocate the extra 0-byte for sdp_data_t->val.str to ensure this
overrun can't happen.

Co-developed-by: Zander Brown <zbrown@gnome.org>
---
 lib/sdp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/sdp.c b/lib/sdp.c
index 844ae0d25..1565259a3 100644
--- a/lib/sdp.c
+++ b/lib/sdp.c
@@ -420,7 +420,7 @@ sdp_data_t *sdp_data_alloc_with_length(uint8_t dtd, const void *value,
 
 		d->unitSize += length;
 		if (length <= USHRT_MAX) {
-			d->val.str = malloc(length);
+			d->val.str = bt_malloc0(length + 1);
 			if (!d->val.str) {
 				free(d);
 				return NULL;
-- 
2.41.0

RE: Fix an allocation oversight in SDP parsing

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 8293 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=798710

---Test result---

Test Summary:
CheckPatch                    FAIL      1.64 seconds
GitLint                       PASS      0.91 seconds
BuildEll                      PASS      33.44 seconds
BluezMake                     PASS      953.81 seconds
MakeCheck                     PASS      12.88 seconds
MakeDistcheck                 PASS      200.22 seconds
CheckValgrind                 PASS      309.63 seconds
CheckSmatch                   PASS      413.84 seconds
bluezmakeextell               PASS      135.45 seconds
IncrementalBuild              PASS      3258.43 seconds
ScanBuild                     WARNING   1227.60 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ,1/4] lib/sdp: Allocate strings in sdp_data_t with NULL termination
WARNING:BAD_SIGN_OFF: Co-developed-by: must be immediately followed by Signed-off-by:
#59: 
Co-developed-by: Zander Brown <zbrown@gnome.org>
---
/github/workspace/src/src/13444881.patch total: 0 errors, 1 warnings, 8 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13444881.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


[BlueZ,2/4] lib/sdp: Don't assume uint8_t has size 1
WARNING:REPEATED_WORD: Possible repeated word: 'of'
#47: 
Assuming the size of of uint8_t is bad practice, we use

WARNING:BAD_SIGN_OFF: Co-developed-by: must be immediately followed by Signed-off-by:
#52: 
Co-developed-by: Zander Brown <zbrown@gnome.org>
---
/github/workspace/src/src/13444882.patch total: 0 errors, 2 warnings, 8 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13444882.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


[BlueZ,3/4] lib/sdp: Use correct string length in sdp_copy_seq()
WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#52: 
only the length of the string (so `sdp_data_t->unitSize - sizeof(uint8_t)`).

WARNING:BAD_SIGN_OFF: Co-developed-by: must be immediately followed by Signed-off-by:
#61: 
Co-developed-by: Zander Brown <zbrown@gnome.org>
---
/github/workspace/src/src/13444883.patch total: 0 errors, 2 warnings, 13 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13444883.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


[BlueZ,4/4] lib/sdp: Pass size_t to sdp_get_string_attr()
WARNING:BAD_SIGN_OFF: Co-developed-by: must be immediately followed by Signed-off-by:
#58: 
Co-developed-by: Zander Brown <zbrown@gnome.org>
---
WARNING:LONG_LINE_COMMENT: line length of 91 exceeds 80 columns
#80: FILE: lib/sdp.c:2189:
+			/* Have to copy the NULL terminator too, so check len < valuelen */

WARNING:LONG_LINE: line length of 94 exceeds 80 columns
#94: FILE: lib/sdp_lib.h:144:
+int sdp_get_string_attr(const sdp_record_t *rec, uint16_t attr, char *value, size_t valuelen);

WARNING:LONG_LINE: line length of 86 exceeds 80 columns
#103: FILE: lib/sdp_lib.h:546:
+static inline int sdp_get_service_name(const sdp_record_t *rec, char *str, size_t len)

WARNING:LONG_LINE: line length of 86 exceeds 80 columns
#109: FILE: lib/sdp_lib.h:551:
+static inline int sdp_get_service_desc(const sdp_record_t *rec, char *str, size_t len)

WARNING:LONG_LINE: line length of 87 exceeds 80 columns
#115: FILE: lib/sdp_lib.h:556:
+static inline int sdp_get_provider_name(const sdp_record_t *rec, char *str, size_t len)

WARNING:LONG_LINE: line length of 81 exceeds 80 columns
#121: FILE: lib/sdp_lib.h:561:
+static inline int sdp_get_doc_url(const sdp_record_t *rec, char *str, size_t len)

WARNING:LONG_LINE: line length of 87 exceeds 80 columns
#127: FILE: lib/sdp_lib.h:566:
+static inline int sdp_get_clnt_exec_url(const sdp_record_t *rec, char *str, size_t len)

WARNING:LONG_LINE: line length of 82 exceeds 80 columns
#133: FILE: lib/sdp_lib.h:571:
+static inline int sdp_get_icon_url(const sdp_record_t *rec, char *str, size_t len)

/github/workspace/src/src/13444884.patch total: 0 errors, 9 warnings, 62 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13444884.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: ScanBuild - WARNING
Desc: Run Scan Build
Output:
lib/sdp.c:507:16: warning: Dereference of undefined pointer value
                int8_t dtd = *(uint8_t *) dtds[i];
                             ^~~~~~~~~~~~~~~~~~~~
lib/sdp.c:535:17: warning: Dereference of undefined pointer value
                uint8_t dtd = *(uint8_t *) dtds[i];
                              ^~~~~~~~~~~~~~~~~~~~
lib/sdp.c:580:12: warning: Access to field 'attrId' results in a dereference of a null pointer (loaded from variable 'd')
        d->attrId = attr;
        ~         ^
lib/sdp.c:1870:26: warning: Potential leak of memory pointed to by 'ap'
        for (; pdlist; pdlist = pdlist->next) {
                                ^~~~~~
lib/sdp.c:1884:6: warning: Potential leak of memory pointed to by 'pds'
                ap = sdp_list_append(ap, pds);
                ~~~^~~~~~~~~~~~~~~~~~~~~~~~~~
lib/sdp.c:1929:10: warning: Potential leak of memory pointed to by 'u'
                        *seqp = sdp_list_append(*seqp, u);
                        ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
lib/sdp.c:2034:4: warning: Potential leak of memory pointed to by 'lang'
                        sdp_list_free(*langSeq, free);
                        ^~~~~~~~~~~~~
lib/sdp.c:2123:9: warning: Potential leak of memory pointed to by 'profDesc'
        return 0;
               ^
lib/sdp.c:3251:8: warning: Potential leak of memory pointed to by 'pSvcRec'
                pSeq = sdp_list_append(pSeq, pSvcRec);
                ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
lib/sdp.c:3252:9: warning: Potential leak of memory pointed to by 'pSeq'
                pdata += sizeof(uint32_t);
                ~~~~~~^~~~~~~~~~~~~~~~~~~
lib/sdp.c:4588:13: warning: Potential leak of memory pointed to by 'rec_list'
                        } while (scanned < attr_list_len && pdata_len > 0);
                                 ^~~~~~~
lib/sdp.c:4884:40: warning: Potential leak of memory pointed to by 'tseq'
        for (d = sdpdata->val.dataseq; d; d = d->next) {
                                              ^
lib/sdp.c:4920:8: warning: Potential leak of memory pointed to by 'subseq'
                tseq = sdp_list_append(tseq, subseq);
                ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
13 warnings generated.



---
Regards,
Linux Bluetooth

[PATCH BlueZ 2/4] lib/sdp: Don't assume uint8_t has size 1

[Jonas Dreßler]

Assuming the size of of uint8_t is bad practice, we use
sizeof(uint8_t) everywhere else and the use of sizeof makes it clear
we're accounting for the descriptor here rather than just randomly
subtracting 1, so change that.

Co-developed-by: Zander Brown <zbrown@gnome.org>
---
 lib/sdp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/sdp.c b/lib/sdp.c
index 1565259a3..006ab057a 100644
--- a/lib/sdp.c
+++ b/lib/sdp.c
@@ -1505,7 +1505,7 @@ static void *sdp_data_value(sdp_data_t *data, uint32_t *len)
 	case SDP_TEXT_STR32:
 		val = data->val.str;
 		if (len)
-			*len = data->unitSize - 1;
+			*len = data->unitSize - sizeof(uint8_t);
 		break;
 	case SDP_ALT8:
 	case SDP_ALT16:
-- 
2.41.0

[PATCH BlueZ 3/4] lib/sdp: Use correct string length in sdp_copy_seq()

[Jonas Dreßler]

sdp_data_t->unitSize for strings in the SDP record is
`sizeof(uint8_t) + strlen(str)`.

The "length" argument of sdp_data_alloc_with_length() is expected to be
only the length of the string (so `sdp_data_t->unitSize - sizeof(uint8_t)`).

Since the last commit, in sdp_copy_seq() we're allocating one byte too much
for strings now, because the `sizeof(uint8_t)` is not subtracted from unitSize
there.

Fix this by making use of the length returned by sdp_data_value() and pass
that on to sdp_data_alloc_with_length().

Co-developed-by: Zander Brown <zbrown@gnome.org>
---
 lib/sdp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/sdp.c b/lib/sdp.c
index 006ab057a..4b10d8f67 100644
--- a/lib/sdp.c
+++ b/lib/sdp.c
@@ -1527,10 +1527,10 @@ static sdp_data_t *sdp_copy_seq(sdp_data_t *data)
 	for (tmp = data; tmp; tmp = tmp->next) {
 		sdp_data_t *datatmp;
 		void *value;
+		uint32_t len = 0;
 
-		value = sdp_data_value(tmp, NULL);
-		datatmp = sdp_data_alloc_with_length(tmp->dtd, value,
-								tmp->unitSize);
+		value = sdp_data_value(tmp, &len);
+		datatmp = sdp_data_alloc_with_length(tmp->dtd, value, len);
 
 		if (cur)
 			cur->next = datatmp;
-- 
2.41.0

[PATCH BlueZ 4/4] lib/sdp: Pass size_t to sdp_get_string_attr()

[Jonas Dreßler]

We're currently type-casting the output of strlen(sdpdata->val.str) into
an int, which is somewhat problematic given that strlen() can return
values larger than sizeof(int).

We can do better here and use size_t instead, so let's do that.

While at it, also add a comment explaining why the check here is "smaller
than" instead of "smaller than or equal".

Co-developed-by: Zander Brown <zbrown@gnome.org>
---
 lib/sdp.c     |  5 +++--
 lib/sdp_lib.h | 14 +++++++-------
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/lib/sdp.c b/lib/sdp.c
index 4b10d8f67..cff7e09fb 100644
--- a/lib/sdp.c
+++ b/lib/sdp.c
@@ -2180,13 +2180,14 @@ int sdp_get_int_attr(const sdp_record_t *rec, uint16_t attrid, int *value)
 }
 
 int sdp_get_string_attr(const sdp_record_t *rec, uint16_t attrid, char *value,
-								int valuelen)
+								size_t valuelen)
 {
 	sdp_data_t *sdpdata = sdp_data_get(rec, attrid);
 	if (sdpdata)
 		/* Verify that it is what the caller expects */
 		if (SDP_IS_TEXT_STR(sdpdata->dtd))
-			if ((int) strlen(sdpdata->val.str) < valuelen) {
+			/* Have to copy the NULL terminator too, so check len < valuelen */
+			if (strlen(sdpdata->val.str) < valuelen) {
 				strcpy(value, sdpdata->val.str);
 				return 0;
 			}
diff --git a/lib/sdp_lib.h b/lib/sdp_lib.h
index 22776b678..91d46f59d 100644
--- a/lib/sdp_lib.h
+++ b/lib/sdp_lib.h
@@ -141,7 +141,7 @@ int sdp_general_inquiry(inquiry_info *ii, int dev_num, int duration, uint8_t *fo
 
 /* flexible extraction of basic attributes - Jean II */
 int sdp_get_int_attr(const sdp_record_t *rec, uint16_t attr, int *value);
-int sdp_get_string_attr(const sdp_record_t *rec, uint16_t attr, char *value, int valuelen);
+int sdp_get_string_attr(const sdp_record_t *rec, uint16_t attr, char *value, size_t valuelen);
 
 /*
  * Basic sdp data functions
@@ -543,32 +543,32 @@ int sdp_get_service_avail(const sdp_record_t *rec, uint8_t *svcAvail);
 int sdp_get_service_ttl(const sdp_record_t *rec, uint32_t *svcTTLInfo);
 int sdp_get_database_state(const sdp_record_t *rec, uint32_t *svcDBState);
 
-static inline int sdp_get_service_name(const sdp_record_t *rec, char *str, int len)
+static inline int sdp_get_service_name(const sdp_record_t *rec, char *str, size_t len)
 {
 	return sdp_get_string_attr(rec, SDP_ATTR_SVCNAME_PRIMARY, str, len);
 }
 
-static inline int sdp_get_service_desc(const sdp_record_t *rec, char *str, int len)
+static inline int sdp_get_service_desc(const sdp_record_t *rec, char *str, size_t len)
 {
 	return sdp_get_string_attr(rec, SDP_ATTR_SVCDESC_PRIMARY, str, len);
 }
 
-static inline int sdp_get_provider_name(const sdp_record_t *rec, char *str, int len)
+static inline int sdp_get_provider_name(const sdp_record_t *rec, char *str, size_t len)
 {
 	return sdp_get_string_attr(rec, SDP_ATTR_PROVNAME_PRIMARY, str, len);
 }
 
-static inline int sdp_get_doc_url(const sdp_record_t *rec, char *str, int len)
+static inline int sdp_get_doc_url(const sdp_record_t *rec, char *str, size_t len)
 {
 	return sdp_get_string_attr(rec, SDP_ATTR_DOC_URL, str, len);
 }
 
-static inline int sdp_get_clnt_exec_url(const sdp_record_t *rec, char *str, int len)
+static inline int sdp_get_clnt_exec_url(const sdp_record_t *rec, char *str, size_t len)
 {
 	return sdp_get_string_attr(rec, SDP_ATTR_CLNT_EXEC_URL, str, len);
 }
 
-static inline int sdp_get_icon_url(const sdp_record_t *rec, char *str, int len)
+static inline int sdp_get_icon_url(const sdp_record_t *rec, char *str, size_t len)
 {
 	return sdp_get_string_attr(rec, SDP_ATTR_ICON_URL, str, len);
 }
-- 
2.41.0

Re: [PATCH BlueZ 0/4] Fix an allocation oversight in SDP parsing

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Fri,  3 Nov 2023 19:21:46 +0100 you wrote:
> There's fairly old oversight in the SDP parsing code where it was forgotten to
> add a NULL termination byte to strings that are later handled using strlen().
> 
> This series fixes that oversight, with a few commits to better follow best
> practices on top.
> 
> Found by running with address sanitizer.
> 
> [...]

Here is the summary with links:
  - [BlueZ,1/4] lib/sdp: Allocate strings in sdp_data_t with NULL termination
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=fdb5ba2cbff3
  - [BlueZ,2/4] lib/sdp: Don't assume uint8_t has size 1
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=cfcc6346a96a
  - [BlueZ,3/4] lib/sdp: Use correct string length in sdp_copy_seq()
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=5afa25c95e6a
  - [BlueZ,4/4] lib/sdp: Pass size_t to sdp_get_string_attr()
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html