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:
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 | 5 +++--
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, 32 insertions(+), 13 deletions(-)
--
2.35.1
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
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
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
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
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 | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/monitor/packet.c b/monitor/packet.c
index b7431b57d..15d629e2d 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);
@@ -2833,7 +2834,7 @@ static void print_le_states(const uint8_t *states_array)
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
If device is no longer exists or not paired when notifications send it
is possible to get double free and dereference of already freed memory.
To avoid this we need to recheck the state of device after sending
notification.
Found by Linux Verification Center (linuxtesting.org) with the SVACE
static analysis tool.
---
src/gatt-database.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/gatt-database.c b/src/gatt-database.c
index d6c94058c..d32f616a9 100644
--- a/src/gatt-database.c
+++ b/src/gatt-database.c
@@ -3877,6 +3877,10 @@ void btd_gatt_database_server_connected(struct btd_gatt_database *database,
send_notification_to_device(state, state->pending);
+ state = find_device_state(database, &bdaddr, bdaddr_type);
+ if (!state || !state->pending)
+ return;
+
free(state->pending->value);
free(state->pending);
state->pending = NULL;
--
2.35.1
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
Hello:
This series was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <[email protected]>:
On Fri, 1 Apr 2022 13:27:50 +0300 you wrote:
> 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.
>
> [...]
Here is the summary with links:
- [BlueZ,1/7] monitor: Fix out-of-bound read in print_le_states
(no matching commit)
- [BlueZ,2/7] tools: Fix buffer overflow in hciattach_tialt.c
https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=d328abaa1715
- [BlueZ,3/7] tools: Fix signed integer overflow in btsnoop.c
https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=497a0b220dbd
- [BlueZ,4/7] tools: Prevent infinity loops in bluemoon.c
(no matching commit)
- [BlueZ,5/7] tools: Limit width of fields in sscanf
https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=1e664b9838cd
- [BlueZ,6/7] device: Limit width of fields in sscanf
https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=4afbef7790ce
- [BlueZ,7/7] gatt: Fix double free and freed memory dereference
https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=3627eddea130
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html