2022-04-01 14:05:08

by Ildar Kamaletdinov

[permalink] [raw]
Subject: [PATCH BlueZ 0/7] [v3] Fix bugs found by SVACE static analisys tool

This patch set includes few fixes that was found by Linux Verification Center
(linuxtesting.org) with the SVACE static analysis tool.

I have manually filtered out non-relevant and false positive problems and only
procedeed with bugs that currently lead to some errors/vulnerabilities or may
lead to them in some specific conditions.

Changelog:
v3 one fix wasn't staged, sorry, one more fix after CI checks
v2 some minor style fixes after CI check.
v1 initial version.

Ildar Kamaletdinov (7):
monitor: Fix out-of-bound read in print_le_states
tools: Fix buffer overflow in hciattach_tialt.c
tools: Fix signed integer overflow in btsnoop.c
tools: Prevent infinity loops in bluemoon.c
tools: Limit width of fields in sscanf
device: Limit width of fields in sscanf
gatt: Fix double free and freed memory dereference

monitor/packet.c | 7 ++++---
src/device.c | 14 +++++++-------
src/gatt-database.c | 4 ++++
tools/bluemoon.c | 13 +++++++++++++
tools/btmgmt.c | 2 +-
tools/btsnoop.c | 2 +-
tools/hciattach_tialt.c | 3 ++-
tools/hex2hcd.c | 2 +-
8 files changed, 33 insertions(+), 14 deletions(-)

--
2.35.1


2022-04-01 14:37:32

by Ildar Kamaletdinov

[permalink] [raw]
Subject: [PATCH BlueZ 2/7] tools: Fix buffer overflow in hciattach_tialt.c

Array 'c_brf_chip' of size 8 could be accessed by index > 7. We should
limit array access like in previous check at line 221.

Found by Linux Verification Center (linuxtesting.org) with the SVACE
static analysis tool.
---
tools/hciattach_tialt.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/hciattach_tialt.c b/tools/hciattach_tialt.c
index 520b383a1..4f7fd42a3 100644
--- a/tools/hciattach_tialt.c
+++ b/tools/hciattach_tialt.c
@@ -221,7 +221,8 @@ int texasalt_init(int fd, int speed, struct termios *ti)
((brf_chip > 7) ? "unknown" : c_brf_chip[brf_chip]),
brf_chip);

- sprintf(fw, "/etc/firmware/%s.bin", c_brf_chip[brf_chip]);
+ sprintf(fw, "/etc/firmware/%s.bin",
+ (brf_chip > 7) ? "unknown" : c_brf_chip[brf_chip]);
texas_load_firmware(fd, fw);

texas_change_speed(fd, speed);
--
2.35.1

2022-04-01 15:29:47

by Ildar Kamaletdinov

[permalink] [raw]
Subject: [PATCH BlueZ 1/7] monitor: Fix out-of-bound read in print_le_states

Accessing le_states_desc_table array with value 15 can cause
out-of-bound read because current size of array is 14.

Currently this cannot lead to any problems becase we do no have such
state in le_states_comb_table but this could be changed in future and
raise described problem.

Found by Linux Verification Center (linuxtesting.org) with the SVACE
static analysis tool.
---
monitor/packet.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/monitor/packet.c b/monitor/packet.c
index b7431b57d..1f04063d3 100644
--- a/monitor/packet.c
+++ b/monitor/packet.c
@@ -2816,7 +2816,8 @@ static const struct {
static void print_le_states(const uint8_t *states_array)
{
uint64_t mask, states = 0;
- int i, n;
+ int i = 0;
+ size_t n = 0;

for (i = 0; i < 8; i++)
states |= ((uint64_t) states_array[i]) << (i * 8);
@@ -2828,12 +2829,12 @@ static void print_le_states(const uint8_t *states_array)
for (i = 0; le_states_comb_table[i].states; i++) {
uint64_t val = (((uint64_t) 1) << le_states_comb_table[i].bit);
const char *str[3] = { NULL, };
- int num = 0;
+ size_t num = 0;

if (!(states & val))
continue;

- for (n = 0; n < 16; n++) {
+ for (n = 0; n < ARRAY_SIZE(le_states_desc_table); n++) {
if (le_states_comb_table[i].states & (1 << n))
str[num++] = le_states_desc_table[n].str;
}
--
2.35.1

2022-04-01 15:52:00

by Ildar Kamaletdinov

[permalink] [raw]
Subject: [PATCH BlueZ 3/7] tools: Fix signed integer overflow in btsnoop.c

If malformed packet is proceed with zero 'size' field we will face with
wrong behaviour of write() call. Value 'toread - 1' gives wrong sign
for value 'written' (-1) in write() call. To prevent this we should
check that 'toread' is not equal to zero.

Found by Linux Verification Center (linuxtesting.org) with the SVACE
static analysis tool.
---
tools/btsnoop.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/btsnoop.c b/tools/btsnoop.c
index 738027dfc..a0d6cf356 100644
--- a/tools/btsnoop.c
+++ b/tools/btsnoop.c
@@ -193,7 +193,7 @@ next_packet:
flags = be32toh(input_pkt[select_input].flags);

len = read(input_fd[select_input], buf, toread);
- if (len < 0 || len != (ssize_t) toread) {
+ if (toread == 0 || len < 0 || len != (ssize_t) toread) {
close(input_fd[select_input]);
input_fd[select_input] = -1;
goto next_packet;
--
2.35.1

2022-04-01 20:28:00

by Ildar Kamaletdinov

[permalink] [raw]
Subject: [PATCH BlueZ 6/7] device: Limit width of fields in sscanf

In src/device.c few sscanf does not limit width of uuid field. This
could lead to static overflow and stack corruption.

Found by Linux Verification Center (linuxtesting.org) with the SVACE
static analysis tool.
---
src/device.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/device.c b/src/device.c
index 381faf91c..8dc12d026 100644
--- a/src/device.c
+++ b/src/device.c
@@ -3790,8 +3790,8 @@ static int load_desc(char *handle, char *value,
return -EIO;

/* Check if there is any value stored, otherwise it is just the UUID */
- if (sscanf(value, "%04hx:%s", &val, uuid_str) != 2) {
- if (sscanf(value, "%s", uuid_str) != 1)
+ if (sscanf(value, "%04hx:%36s", &val, uuid_str) != 2) {
+ if (sscanf(value, "%36s", uuid_str) != 1)
return -EIO;
val = 0;
}
@@ -3840,9 +3840,9 @@ static int load_chrc(char *handle, char *value,
return -EIO;

/* Check if there is any value stored */
- if (sscanf(value, GATT_CHARAC_UUID_STR ":%04hx:%02hx:%32s:%s",
+ if (sscanf(value, GATT_CHARAC_UUID_STR ":%04hx:%02hx:%32s:%36s",
&value_handle, &properties, val_str, uuid_str) != 4) {
- if (sscanf(value, GATT_CHARAC_UUID_STR ":%04hx:%02hx:%s",
+ if (sscanf(value, GATT_CHARAC_UUID_STR ":%04hx:%02hx:%36s",
&value_handle, &properties, uuid_str) != 3)
return -EIO;
val_len = 0;
@@ -3884,8 +3884,8 @@ static int load_incl(struct gatt_db *db, char *handle, char *value,
if (sscanf(handle, "%04hx", &start) != 1)
return -EIO;

- if (sscanf(value, GATT_INCLUDE_UUID_STR ":%04hx:%04hx:%s", &start, &end,
- uuid_str) != 3)
+ if (sscanf(value, GATT_INCLUDE_UUID_STR ":%04hx:%04hx:%36s", &start,
+ &end, uuid_str) != 3)
return -EIO;

/* Log debug message. */
@@ -3918,7 +3918,7 @@ static int load_service(struct gatt_db *db, char *handle, char *value)
if (sscanf(handle, "%04hx", &start) != 1)
return -EIO;

- if (sscanf(value, "%[^:]:%04hx:%s", type, &end, uuid_str) != 3)
+ if (sscanf(value, "%[^:]:%04hx:%36s", type, &end, uuid_str) != 3)
return -EIO;

if (g_str_equal(type, GATT_PRIM_SVC_UUID_STR))
--
2.35.1

2022-04-02 10:28:18

by Ildar Kamaletdinov

[permalink] [raw]
Subject: [PATCH BlueZ 4/7] tools: Prevent infinity loops in bluemoon.c

In case FW size is too big we can face with infinity while() loops.
According to C99 standard SIZE_MAX could be as small as 65535.

So to prevent overflow of 'firmware_offset' we must limit maximum FW
size that could be processed by bluemoon.

Found by Linux Verification Center (linuxtesting.org) with the SVACE
static analysis tool.
---
tools/bluemoon.c | 13 +++++++++++++
1 file changed, 13 insertions(+)

diff --git a/tools/bluemoon.c b/tools/bluemoon.c
index f50107a2a..729da36f6 100644
--- a/tools/bluemoon.c
+++ b/tools/bluemoon.c
@@ -492,6 +492,13 @@ static void request_firmware(const char *path)
return;
}

+ if (st.st_size > (SIZE_MAX - 4)) {
+ fprintf(stderr, "Firmware size is too big\n");
+ close(fd);
+ shutdown_device();
+ return;
+ }
+
firmware_data = malloc(st.st_size);
if (!firmware_data) {
fprintf(stderr, "Failed to allocate firmware buffer\n");
@@ -874,6 +881,12 @@ static void analyze_firmware(const char *path)
return;
}

+ if (st.st_size > (SIZE_MAX - 3)) {
+ fprintf(stderr, "Firmware size is too big\n");
+ close(fd);
+ return;
+ }
+
firmware_data = malloc(st.st_size);
if (!firmware_data) {
fprintf(stderr, "Failed to allocate firmware buffer\n");
--
2.35.1

2022-04-02 14:38:12

by Ildar Kamaletdinov

[permalink] [raw]
Subject: [PATCH BlueZ 5/7] tools: Limit width of fields in sscanf

In tools/btmgmt.c and tools/hex2hcd.c few sscanf does not limit width
of fields. This could lead to static overflow and stack corruption.

Found by Linux Verification Center (linuxtesting.org) with the SVACE
static analysis tool.
---
tools/btmgmt.c | 2 +-
tools/hex2hcd.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/btmgmt.c b/tools/btmgmt.c
index 42ef9acef..8f63f12ba 100644
--- a/tools/btmgmt.c
+++ b/tools/btmgmt.c
@@ -5164,7 +5164,7 @@ static bool str2pattern(struct mgmt_adv_pattern *pattern, const char *str)
char pattern_str[62] = { 0 };
char tmp;

- if (sscanf(str, "%2hhx%n:%2hhx%n:%s", &pattern->ad_type, &type_len,
+ if (sscanf(str, "%2hhx%n:%2hhx%n:%61s", &pattern->ad_type, &type_len,
&pattern->offset, &offset_end_pos, pattern_str) != 3)
return false;

diff --git a/tools/hex2hcd.c b/tools/hex2hcd.c
index 674d62744..e6dca5a81 100644
--- a/tools/hex2hcd.c
+++ b/tools/hex2hcd.c
@@ -248,7 +248,7 @@ static void ver_parse_file(const char *pathname)

memset(ver, 0, sizeof(*ver));

- if (sscanf(pathname, "%[A-Z0-9]_%3c.%3c.%3c.%4c.%4c.hex",
+ if (sscanf(pathname, "%19[A-Z0-9]_%3c.%3c.%3c.%4c.%4c.hex",
ver->name, ver->major, ver->minor,
ver->build, dummy1, dummy2) != 6) {
printf("\t/* failed to parse %s */\n", pathname);
--
2.35.1