On 5/3/21 9:51 AM, Greg Kroah-Hartman wrote:
On Mon, May 03, 2021 at 08:17:14AM -0500, Mark Langsdorf wrote:
In 5/2/21 12:23 PM, Kees Cook wrote:
This reverts commit 03d1571d9513369c17e6848476763ebbd10ec2cb.
While /sys/kernel/debug/acpi/custom_method is already a privileged-only
API providing proxied arbitrary write access to kernel memory[1][2],
with existing race conditions[3] in buffer allocation and use that could
lead to memory leaks and use-after-free conditions, the above commit
appears to accidentally make the use-after-free conditions even easier
to accomplish. ("buf" is a global variable and prior kfree()s would set
buf back to NULL.)
This entire interface needs to be reworked (if not entirely removed).
[1] https://lore.kernel.org/lkml/20110222193250.GA23913@xxxxxxxxxxx/
[2] https://lore.kernel.org/lkml/201906221659.B618D83@keescook/
[3] https://lore.kernel.org/lkml/20170109231323.GA89642@beast/
Cc: Wenwen Wang <wenwen@xxxxxxxxxx>
Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
---
I have two patches submitted to linux-acpi to fix the most obvious bugs in
the current driver. I don't think that just reverting this patch in its
entirety is a good solution: it still leaves the buf allocated in -EINVAL,
as well as the weird case where a not fully consumed buffer can be
reallocated without being freed on a subsequent call.
https://lore.kernel.org/linux-acpi/20210427185434.34885-1-mlangsdo@xxxxxxxxxx/
https://lore.kernel.org/linux-acpi/20210423152818.97077-1-mlangsdo@xxxxxxxxxx/
I support rewriting this driver in its entirety, but reverting one bad patch
to leave it in a different buggy state is less than ideal.
It's buggy now, and root-only, so it's a low bar at the moment :)
Do those commits really fix the issues? Is this debugfs code even
needed at all or can it just be dropped?
One of my commits removes the kfree(buf) at the end of the function,
which is the code that causes the use after free for short writes. The
other adds a kfree(buf) before allocating the buffer, to make sure that
the buffer is free before allocating it.
There are other bugs in the code that neither my patches nor the revert
address, like the total lack of protection against concurrent writes.
--Mark Langsdorf
