[PATCH BlueZ v1 0/4] fix errors found by SVACE static analyzer #3

[PATCH BlueZ v1 0/4] fix errors found by SVACE static analyzer #3

[Roman Smirnov]

Several bug fixes.

Roman Smirnov (4):
  health: mcap: add checks for NULL mcap_notify_error()
  shared: prevent dereferencing of NULL pointers
  settings: limit the string size in load_service()
  settings: limit the number of chars to be read in gatt_db_load()

 profiles/health/mcap.c |  9 +++++++
 src/settings.c         | 58 +++++++++++++++++++++++++++++++++++++++---
 src/shared/micp.c      |  4 +++
 src/shared/vcp.c       | 12 +++++++++
 4 files changed, 80 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH BlueZ v1 1/4] health: mcap: add checks for NULL mcap_notify_error()

[Roman Smirnov]

It is necessary to prevent dereferencing of NULL pointers.

Found with the SVACE static analysis tool.
---
 profiles/health/mcap.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/profiles/health/mcap.c b/profiles/health/mcap.c
index 7eceaa88a..2e4214a69 100644
--- a/profiles/health/mcap.c
+++ b/profiles/health/mcap.c
@@ -336,6 +336,9 @@ static void mcap_notify_error(struct mcap_mcl *mcl, GError *err)
 	case MCAP_MD_CREATE_MDL_REQ:
 		st = MDL_WAITING;
 		l = g_slist_find_custom(mcl->mdls, &st, cmp_mdl_state);
+		if (!l)
+			return;
+
 		mdl = l->data;
 		mcl->mdls = g_slist_remove(mcl->mdls, mdl);
 		mcap_mdl_unref(mdl);
@@ -345,6 +348,9 @@ static void mcap_notify_error(struct mcap_mcl *mcl, GError *err)
 	case MCAP_MD_ABORT_MDL_REQ:
 		st = MDL_WAITING;
 		l = g_slist_find_custom(mcl->mdls, &st, cmp_mdl_state);
+		if (!l)
+			return;
+
 		shutdown_mdl(l->data);
 		update_mcl_state(mcl);
 		con->cb.notify(err, con->user_data);
@@ -362,6 +368,9 @@ static void mcap_notify_error(struct mcap_mcl *mcl, GError *err)
 	case MCAP_MD_RECONNECT_MDL_REQ:
 		st = MDL_WAITING;
 		l = g_slist_find_custom(mcl->mdls, &st, cmp_mdl_state);
+		if (!l)
+			return;
+
 		shutdown_mdl(l->data);
 		update_mcl_state(mcl);
 		con->cb.op(NULL, err, con->user_data);
-- 
2.34.1

[PATCH BlueZ v1 2/4] shared: prevent dereferencing of NULL pointers

[Roman Smirnov]

It is necessary to add checks for NULL before dereferencing pointers.

Found with the SVACE static analysis tool.
---
 src/shared/micp.c |  4 ++++
 src/shared/vcp.c  | 12 ++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/src/shared/micp.c b/src/shared/micp.c
index b82bd92de..1c34e9d00 100644
--- a/src/shared/micp.c
+++ b/src/shared/micp.c
@@ -398,6 +398,10 @@ static void mics_mute_write(struct gatt_db_attribute *attrib,
 	}
 
 	micp_op = iov_pull_mem(&iov, sizeof(*micp_op));
+	if (!micp_op) {
+		DBG(micp, "iov_pull_mem() returned NULL");
+		goto respond;
+	}
 
 	if ((*micp_op == MICS_DISABLED) || (*micp_op != MICS_NOT_MUTED
 		&& *micp_op != MICS_MUTED)) {
diff --git a/src/shared/vcp.c b/src/shared/vcp.c
index 06264a241..602d46dc1 100644
--- a/src/shared/vcp.c
+++ b/src/shared/vcp.c
@@ -925,6 +925,10 @@ static void vcs_cp_write(struct gatt_db_attribute *attrib,
 	}
 
 	vcp_op = iov_pull_mem(&iov, sizeof(*vcp_op));
+	if (!vcp_op) {
+		DBG(vcp, "iov_pull_mem() returned NULL");
+		goto respond;
+	}
 
 	for (handler = vcp_handlers; handler && handler->str; handler++) {
 		if (handler->op != *vcp_op)
@@ -985,6 +989,10 @@ static void vocs_cp_write(struct gatt_db_attribute *attrib,
 	}
 
 	vcp_op = iov_pull_mem(&iov, sizeof(*vcp_op));
+	if (!vcp_op) {
+		DBG(vcp, "iov_pull_mem() returned NULL");
+		goto respond;
+	}
 
 	for (handler = vocp_handlers; handler && handler->str; handler++) {
 		if (handler->op != *vcp_op)
@@ -1517,6 +1525,10 @@ static void aics_ip_cp_write(struct gatt_db_attribute *attrib,
 	}
 
 	aics_op = iov_pull_mem(&iov, sizeof(*aics_op));
+	if (!aics_op) {
+		DBG(vcp, "iov_pull_mem() returned NULL");
+		goto respond;
+	}
 
 	for (handler = aics_handlers; handler && handler->str; handler++) {
 		if (handler->op != *aics_op)
-- 
2.34.1

[PATCH BlueZ v1 3/4] settings: limit the string size in load_service()

[Roman Smirnov]

Calculate the length of the first string and use it to create
a pattern. The pattern will limit the maximum length of the
string, which will prevent the buffer from overflowing.

Found with the SVACE static analysis tool.
---
 src/settings.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/src/settings.c b/src/settings.c
index b61e694f1..4eccf0b4e 100644
--- a/src/settings.c
+++ b/src/settings.c
@@ -187,13 +187,30 @@ static int load_service(struct gatt_db *db, char *handle, char *value)
 	char type[MAX_LEN_UUID_STR], uuid_str[MAX_LEN_UUID_STR];
 	bt_uuid_t uuid;
 	bool primary;
+	char pattern[16];
+	char *colon_pos;
+	size_t len;
 
 	if (sscanf(handle, "%04hx", &start) != 1) {
 		DBG("Failed to parse handle: %s", handle);
 		return -EIO;
 	}
 
-	if (sscanf(value, "%[^:]:%04hx:%36s", type, &end, uuid_str) != 3) {
+	colon_pos = memchr(value, ':', MAX_LEN_UUID_STR);
+	if (!colon_pos) {
+		DBG("Failed to parse value: %s", value);
+		return -EIO;
+	}
+
+	len = colon_pos - value;
+	if (!len) {
+		DBG("Failed to parse value: %s", value);
+		return -EIO;
+	}
+
+	snprintf(pattern, sizeof(pattern), "%%%lds:%%04hx:%%36s", len);
+
+	if (sscanf(value, pattern, type, &end, uuid_str) != 3) {
 		DBG("Failed to parse value: %s", value);
 		return -EIO;
 	}
-- 
2.34.1

[PATCH BlueZ v1 4/4] settings: limit the number of chars to be read in gatt_db_load()

[Roman Smirnov]

It is necessary to limit the string length to prevent buffer overflow.
Find the string length, write it to the pattern and use it for
limiting.

Found with the SVACE static analysis tool.
---
 src/settings.c | 39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/src/settings.c b/src/settings.c
index 4eccf0b4e..dcfbc5601 100644
--- a/src/settings.c
+++ b/src/settings.c
@@ -243,13 +243,32 @@ static int gatt_db_load(struct gatt_db *db, GKeyFile *key_file, char **keys)
 	struct gatt_db_attribute *current_service;
 	char **handle, *value, type[MAX_LEN_UUID_STR];
 	int ret;
+	char pattern[6];
+	char *colon_pos;
+	size_t len;
 
 	/* first load service definitions */
 	for (handle = keys; *handle; handle++) {
 		value = g_key_file_get_string(key_file, "Attributes", *handle,
 									NULL);
+		if (!value)
+			return -EIO;
+
+		colon_pos = memchr(value, ':', MAX_LEN_UUID_STR);
+		if (!colon_pos) {
+			g_free(value);
+			return -EIO;
+		}
+
+		len = colon_pos - value;
+		if (!len) {
+			g_free(value);
+			return -EIO;
+		}
 
-		if (!value || sscanf(value, "%[^:]:", type) != 1) {
+		snprintf(pattern, sizeof(pattern), "%%%lds:", len);
+
+		if (sscanf(value, pattern, type) != 1) {
 			g_free(value);
 			return -EIO;
 		}
@@ -271,8 +290,24 @@ static int gatt_db_load(struct gatt_db *db, GKeyFile *key_file, char **keys)
 	for (handle = keys; *handle; handle++) {
 		value = g_key_file_get_string(key_file, "Attributes", *handle,
 									NULL);
+		if (!value)
+			return -EIO;
+
+		colon_pos = memchr(value, ':', MAX_LEN_UUID_STR);
+		if (!colon_pos) {
+			g_free(value);
+			return -EIO;
+		}
+
+		len = colon_pos - value;
+		if (!len) {
+			g_free(value);
+			return -EIO;
+		}
+
+		snprintf(pattern, sizeof(pattern), "%%%lds:", len);
 
-		if (!value || sscanf(value, "%[^:]:", type) != 1) {
+		if (sscanf(value, pattern, type) != 1) {
 			g_free(value);
 			return -EIO;
 		}
-- 
2.34.1

Re: [PATCH BlueZ v1 3/4] settings: limit the string size in load_service()

[Luiz Augusto von Dentz]

Hi Roman,

On Tue, Jul 9, 2024 at 8:01 AM Roman Smirnov <r.smirnov@omp.ru> wrote:
>
> Calculate the length of the first string and use it to create
> a pattern. The pattern will limit the maximum length of the
> string, which will prevent the buffer from overflowing.
>
> Found with the SVACE static analysis tool.
> ---
>  src/settings.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/src/settings.c b/src/settings.c
> index b61e694f1..4eccf0b4e 100644
> --- a/src/settings.c
> +++ b/src/settings.c
> @@ -187,13 +187,30 @@ static int load_service(struct gatt_db *db, char *handle, char *value)
>         char type[MAX_LEN_UUID_STR], uuid_str[MAX_LEN_UUID_STR];
>         bt_uuid_t uuid;
>         bool primary;
> +       char pattern[16];
> +       char *colon_pos;
> +       size_t len;
>
>         if (sscanf(handle, "%04hx", &start) != 1) {
>                 DBG("Failed to parse handle: %s", handle);
>                 return -EIO;
>         }
>
> -       if (sscanf(value, "%[^:]:%04hx:%36s", type, &end, uuid_str) != 3) {

Can't we just do %36[^:] instead since it is the same size of
uuid_str, the only real difference is that it reads until the ':'
rather than until the end, but %36s is also _at most_ 36 characters.

> +       colon_pos = memchr(value, ':', MAX_LEN_UUID_STR);
> +       if (!colon_pos) {
> +               DBG("Failed to parse value: %s", value);
> +               return -EIO;
> +       }
> +
> +       len = colon_pos - value;
> +       if (!len) {
> +               DBG("Failed to parse value: %s", value);
> +               return -EIO;
> +       }
> +
> +       snprintf(pattern, sizeof(pattern), "%%%lds:%%04hx:%%36s", len);
> +
> +       if (sscanf(value, pattern, type, &end, uuid_str) != 3) {
>                 DBG("Failed to parse value: %s", value);
>                 return -EIO;
>         }
> --
> 2.34.1
>
>


-- 
Luiz Augusto von Dentz

RE: fix errors found by SVACE static analyzer #3

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 949 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=869638

---Test result---

Test Summary:
CheckPatch                    PASS      1.90 seconds
GitLint                       PASS      1.32 seconds
BuildEll                      PASS      24.56 seconds
BluezMake                     PASS      1667.59 seconds
MakeCheck                     PASS      13.11 seconds
MakeDistcheck                 PASS      179.03 seconds
CheckValgrind                 PASS      255.48 seconds
CheckSmatch                   PASS      357.30 seconds
bluezmakeextell               PASS      121.14 seconds
IncrementalBuild              PASS      6078.28 seconds
ScanBuild                     PASS      1030.01 seconds



---
Regards,
Linux Bluetooth

RE: fix errors found by SVACE static analyzer #3

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 949 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=869742

---Test result---

Test Summary:
CheckPatch                    PASS      1.32 seconds
GitLint                       PASS      1.88 seconds
BuildEll                      PASS      25.18 seconds
BluezMake                     PASS      1713.50 seconds
MakeCheck                     PASS      13.36 seconds
MakeDistcheck                 PASS      180.35 seconds
CheckValgrind                 PASS      255.85 seconds
CheckSmatch                   PASS      358.51 seconds
bluezmakeextell               PASS      120.61 seconds
IncrementalBuild              PASS      4642.11 seconds
ScanBuild                     PASS      1036.55 seconds



---
Regards,
Linux Bluetooth