Re: [BUG] [PATCH RFC v2] selftests/firmware: copious kernel memory leaks in test_fw_run_batch_request()
From: Mirsad Todorovac
Date: Thu Mar 30 2023 - 11:15:03 EST
Hi, all,
This is not a formal patch, but please see if you think the way the
locking and race are solved correctly this time.
(Having two mutexes over the same set of resources is obviously a hazard.)
On 3/28/23 12:06, Dan Carpenter wrote:
On Tue, Mar 28, 2023 at 11:23:00AM +0200, Mirsad Todorovac wrote:
The leaks are in chunks of 1024 bytes (+ overhead), but so far I could not
reproduce w/o root privileges, as tests refuse to run as unprivileged user.
(This is not the proof of non-existence of an unprivileged automated exploit
that would exhaust the kernel memory at approx. rate 4 MB/hour on our setup.
This would mean about 96 MB / day or 3 GB / month (of kernel memory).
This is firmware testing stuff. In the real world people aren't going
to run their test scripts in a loop for days.
There is no security implications. This is root only. Also if the
user could load firmware then that would be the headline. Once someone
is can already load firmware then who cares if they leak 100MB per day?
It looks like if you call trigger_batched_requests_store() twice in a
row then it will leak memory. Definitely test_fw_config->reqs is leaked.
That's different from what the bug report is complaining about, but the
point is that there are some obvious leaks. It looks like you're
supposed to call trigger_batched_requests_store() in between runs?
There are other races like config_num_requests_store() should hold the
mutex over the call to test_dev_config_update_u8() instead of dropping
and retaking it.
COMMENT: Like in libc putc() family of functions, there is also
putc_unlocked() The similar approach is applied here.
As the functions are callable from within both locked and non-locked
environment, we have to either:
1. have two or more locks, which is dubious in terms of concurrency
2. have locked and unlocked version of each function, for we cannot
lock the same lock twice.
NOTE: Memory leaks are not solved with this patch, only a couple of
racing conditions.
---
diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index 05ed84c2fc4c..d6ed20bd1eb0 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -353,6 +353,19 @@ static ssize_t config_test_show_str(char *dst,
return len;
}
+static inline int test_dev_config_update_bool_unlocked(const char *buf, size_t size,
+ bool *cfg)
+{
+ int ret;
+
+ if (kstrtobool(buf, cfg) < 0)
+ ret = -EINVAL;
+ else
+ ret = size;
+
+ return ret;
+}
+
static int test_dev_config_update_bool(const char *buf, size_t size,
bool *cfg)
{
@@ -373,6 +386,24 @@ static ssize_t test_dev_config_show_bool(char *buf, bool val)
return snprintf(buf, PAGE_SIZE, "%d\n", val);
}
+static int test_dev_config_update_size_t_unlocked(
+ const char *buf,
+ size_t size,
+ size_t *cfg)
+{
+ int ret;
+ long new;
+
+ ret = kstrtol(buf, 10, &new);
+ if (ret)
+ return ret;
+
+ *(size_t *)cfg = new;
+
+ /* Always return full write size even if we didn't consume all */
+ return size;
+}
+
static int test_dev_config_update_size_t(const char *buf,
size_t size,
size_t *cfg)
@@ -402,6 +433,21 @@ static ssize_t test_dev_config_show_int(char *buf, int val)
return snprintf(buf, PAGE_SIZE, "%d\n", val);
}
+static int test_dev_config_update_u8_unlocked(const char *buf, size_t size, u8 *cfg)
+{
+ u8 val;
+ int ret;
+
+ ret = kstrtou8(buf, 10, &val);
+ if (ret)
+ return ret;
+
+ *(u8 *)cfg = val;
+
+ /* Always return full write size even if we didn't consume all */
+ return size;
+}
+
static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
{
u8 val;
@@ -471,10 +517,10 @@ static ssize_t config_num_requests_store(struct device *dev,
mutex_unlock(&test_fw_mutex);
goto out;
}
- mutex_unlock(&test_fw_mutex);
- rc = test_dev_config_update_u8(buf, count,
- &test_fw_config->num_requests);
+ rc = test_dev_config_update_u8_unlocked(buf, count,
+ &test_fw_config->num_requests);
+ mutex_unlock(&test_fw_mutex);
out:
return rc;
@@ -518,10 +564,10 @@ static ssize_t config_buf_size_store(struct device *dev,
mutex_unlock(&test_fw_mutex);
goto out;
}
- mutex_unlock(&test_fw_mutex);
- rc = test_dev_config_update_size_t(buf, count,
- &test_fw_config->buf_size);
+ rc = test_dev_config_update_size_t_unlocked(buf, count,
+ &test_fw_config->buf_size);
+ mutex_unlock(&test_fw_mutex);
out:
return rc;
@@ -548,10 +594,10 @@ static ssize_t config_file_offset_store(struct device *dev,
mutex_unlock(&test_fw_mutex);
goto out;
}
- mutex_unlock(&test_fw_mutex);
- rc = test_dev_config_update_size_t(buf, count,
- &test_fw_config->file_offset);
+ rc = test_dev_config_update_size_t_unlocked(buf, count,
+ &test_fw_config->file_offset);
+ mutex_unlock(&test_fw_mutex);
out:
return rc;
Best regards,
Mirsad
--
Mirsad Goran Todorovac
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu
System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb, Republic of Croatia
"What’s this thing suddenly coming towards me very fast? Very very fast.
... I wonder if it will be friends with me?"