Re: [PATCH] firmware_loader: Fix use-after-free Read in firmware_loading_store
From: Shuah Khan
Date: Fri Jul 09 2021 - 12:38:17 EST
On 7/9/21 10:15 AM, Shuah Khan wrote:
On 7/9/21 3:17 AM, Hillf Danton wrote:
On Wed, 7 Jul 2021 21:13:21 -0600 Shuah Khan wrote:
If user writes to 'loading' between loading aborted and 'loading'
gets removed, __fw_load_abort() could be called twice in error
path setting the state to load aborted. __fw_load_abort() checks
for fw_sysfs_done() case, but doesn't check for abort case. This
opens the window for use-after-free Read in firmware_loading_store().
Fix it by adding check for fw load aborted in addition to done in
__fw_load_abort() and return if either one of the states is true.
BUG: KASAN: use-after-free in __list_del_entry_valid+0xd6/0xf0 lib/list_debug.c:54
Read of size 8 at addr ffff88802b3da2c8 by task systemd-udevd/25252
CPU: 0 PID: 25252 Comm: systemd-udevd Not tainted 5.13.0-rc1-syzkaller #0
Hardware name: Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:79 [inline]
dump_stack+0x141/0x1d7 lib/dump_stack.c:120
print_address_description.constprop.0.cold+0x5b/0x2f8 mm/kasan/report.c:233
__kasan_report mm/kasan/report.c:419 [inline]
kasan_report.cold+0x7c/0xd8 mm/kasan/report.c:436
__list_del_entry_valid+0xd6/0xf0 lib/list_debug.c:54
__list_del_entry include/linux/list.h:132 [inline]
list_del_init include/linux/list.h:204 [inline]
__fw_load_abort drivers/base/firmware_loader/fallback.c:97 [inline]
__fw_load_abort drivers/base/firmware_loader/fallback.c:88 [inline]
fw_load_abort drivers/base/firmware_loader/fallback.c:105 [inline]
firmware_loading_store+0x141/0x650 drivers/base/firmware_loader/fallback.c:297
dev_attr_store+0x50/0x80 drivers/base/core.c:2066
sysfs_kf_write+0x110/0x160 fs/sysfs/file.c:139
kernfs_fop_write_iter+0x342/0x500 fs/kernfs/file.c:296
call_write_iter include/linux/fs.h:2114 [inline]
new_sync_write+0x426/0x650 fs/read_write.c:518
vfs_write+0x796/0xa30 fs/read_write.c:605
ksys_write+0x12d/0x250 fs/read_write.c:658
do_syscall_64+0x3a/0xb0 arch/x86/entry/common.c:47
entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x7f6d0b3fe970
Code: 73 01 c3 48 8b 0d 28 d5 2b 00 f7 d8 64 89 01 48 83 c8 ff c3 66 0f 1f 44 00 00 83 3d 99 2d 2c 00 00 75 10 b8 01 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 31 c3 48 83 ec 08 e8 7e 9b 01 00 48 89 04 24
RSP: 002b:00007ffde8a82ba8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 00007f6d0b3fe970
RDX: 0000000000000002 RSI: 00005567e595b380 RDI: 0000000000000007
RBP: 00005567e595b380 R08: 00007f6d0c58c8c0 R09: 0000000000000002
R10: 0000000000000020 R11: 0000000000000246 R12: 0000000000000002
R13: 0000000000000001 R14: 00005567e59427d0 R15: 0000000000000002
Reported-by: syzbot+77cea49e091776a57689@xxxxxxxxxxxxxxxxxxxxxxxxx
Signed-off-by: Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx>
---
drivers/base/firmware_loader/fallback.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
index 91899d185e31..e6a18c2a6c43 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -89,9 +89,10 @@ static void __fw_load_abort(struct fw_priv *fw_priv)
{
/*
* There is a small window in which user can write to 'loading'
- * between loading done and disappearance of 'loading'
+ * between loading done or aborted and disappearance of
+ * 'loading'
*/
- if (fw_sysfs_done(fw_priv))
+ if (fw_sysfs_done(fw_priv) || fw_state_is_aborted(fw_priv))
Given the fw_state_is_aborted() in firmware_loading_store(), could you specify
why it is a correct fix to check again?
Yes fw_state_is_aborted() is checked at the beginning of
firmware_loading_store(). Later on you will see that for
for case 0 logic it will do a variation on abort when
failure happens and sets the state to aborted. Note that
this would happen on failure case. Looking at the log from
the report this will be the case as I see errors in loading
path.
/*
* Same logic as fw_load_abort, only the DONE bit
* is ignored and we set ABORT only on failure.
*/
list_del_init(&fw_priv->pending_list);
if (rc) {
fw_state_aborted(fw_priv);
written = rc;
} else {
fw_state_done(fw_priv);
}
break;
If another user writes to the "loading" file before it gets
deleted, if user writes 0 again, fw_sysfs_loading() will be
false, however the fallthrough will invoked fw_load_abort()
and list_del_init() will be called triggering user-after-free
fw_state_aborted() should have already marked the state aborted
and the check at the firmware_loading_store() should catch this
case ... all of this is done with fw_lock lock held ...
However I am seeing the following over and over again in the
log - hence I think it is safer to check the aborted status
in __fw_load_abort().
? __list_del_entry_valid+0xe0/0xf0
[ 348.604808][T12994] __list_del_entry_valid+0xe0/0xf0
[ 348.610020][T12994] firmware_loading_store+0x141/0x650
[ 348.615761][T12994] ? firmware_data_write+0x4e0/0x4e0
[ 348.621064][T12994] ? sysfs_file_ops+0x1c0/0x1c0
[ 348.625921][T12994] dev_attr_store+0x50/0x80
Also the fallback logic takes actions based on errors as in
fw_load_sysfs_fallback() that returns -EAGAIN which would
trigger request_firmware() again.
Based on all of this I think this fix is needed, if only I can
test for sure.
thanks,
-- Shuah