Messages in this thread Patch in this message | | | Date | Thu, 30 Mar 2023 14:14:50 +0200 | Subject | Re: [BUG] [PATCH RFC v1] selftests/firmware: copious kernel memory leaks in test_fw_run_batch_request() | From | Mirsad Todorovac <> |
| |
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,
-- 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?"
| |