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.
Hi Dan,
Following your insight and advice, I tried to mend this racing condition like this:
diff --git a/lib/test_firmware.c b/lib/test_firmware.c index 05ed84c2fc4c..6723c234ccbb 100644 --- a/lib/test_firmware.c +++ b/lib/test_firmware.c @@ -402,6 +402,8 @@ static ssize_t test_dev_config_show_int(char *buf, int val) return snprintf(buf, PAGE_SIZE, "%d\n", val); }
+static DEFINE_MUTEX(test_fw_mutex_update); + static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg) { u8 val; @@ -411,9 +413,9 @@ static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg) if (ret) return ret;
- mutex_lock(&test_fw_mutex); + mutex_lock(&test_fw_mutex_update); *(u8 *)cfg = val; - mutex_unlock(&test_fw_mutex); + mutex_unlock(&test_fw_mutex_update);
/* Always return full write size even if we didn't consume all */ return size; @@ -471,10 +473,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); + mutex_unlock(&test_fw_mutex);
out: return rc;
For the second trigger_batched_requests_store(), probably the desired behaviour would be to extend the list of test_fw_config->reqs, rather than destroying them and allocating the new ones?
I am not certain about the desired semantics and where is it documented.
Thank you.
Best regards,