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: [email protected]
Signed-off-by: Shuah Khan <[email protected]>
---
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))
return;
list_del_init(&fw_priv->pending_list);
--
2.30.2
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: [email protected]
>> Signed-off-by: Shuah Khan <[email protected]>
>> ---
>> 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
Note that in __fw_load_abort() handles this window for loading
done case and doesn't for error case when fw load is aborted.
/*
* There is a small window in which user can write to 'loading'
* between loading done and disappearance of 'loading'
*/
if (fw_sysfs_done(fw_priv))
return;
This fix closes the window for aborted case. However, I can't
test this because there is no reproducer.
thanks,
-- Shuah
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: [email protected]
>>> Signed-off-by: Shuah Khan <[email protected]>
>>> ---
>>> 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
On Fri, Jul 09, 2021 at 10:38:12AM -0600, Shuah Khan wrote:
> 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.
Shuah, curious if you had read this patch from Anirudh Rayabharam
and my response to that v4 patch iteration?
https://lkml.kernel.org/r/[email protected]
Luis
On 7/15/21 4:28 PM, Luis Chamberlain wrote:
> On Fri, Jul 09, 2021 at 10:38:12AM -0600, Shuah Khan wrote:
>> 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.
>
> Shuah, curious if you had read this patch from Anirudh Rayabharam
> and my response to that v4 patch iteration?
>
> https://lkml.kernel.org/r/[email protected]
>
Yes. I realized I am trying to fix the same problem we have been
discussing. :) Sorry for the noise.
Ignore my patch. I will follow the thread.
thanks,
-- Shuah
On Thu, Jul 15, 2021 at 04:46:24PM -0600, Shuah Khan wrote:
> On 7/15/21 4:28 PM, Luis Chamberlain wrote:
> > On Fri, Jul 09, 2021 at 10:38:12AM -0600, Shuah Khan wrote:
> > > 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.
> >
> > Shuah, curious if you had read this patch from Anirudh Rayabharam
> > and my response to that v4 patch iteration?
> >
> > https://lkml.kernel.org/r/[email protected]
> >
>
> Yes. I realized I am trying to fix the same problem we have been
> discussing. :) Sorry for the noise.
No worries, and thanks again for you help!
> Ignore my patch. I will follow the thread.
OK ! I think all we need is just Anirudh to split his patch to
remove the -EAGAIN return value in a separate patch as a first step,
documenting in the commmit log that:
The only motivation on her part with using -EAGAIN on commit
0542ad88fbdd81bb ("firmware loader: Fix _request_firmware_load()
return val for fw load abort") was to distinguish the error from
-ENOMEM, and so there is no real reason in keeping it. Keeping
-ETIMEDOUT is much telling of what the reason for a failure is,
so just use that.
Then his second patch would be simplified without the -EAGAIN
condition.
All I asked was to confirm that the -ETIMEDOUT was indeed propagated.
Anirudh, sorry for the trouble, but can I ask you for a v5 with two
patches as described above?
Luis
On Thu, Jul 15, 2021 at 04:21:05PM -0700, Luis Chamberlain wrote:
> On Thu, Jul 15, 2021 at 04:46:24PM -0600, Shuah Khan wrote:
> > On 7/15/21 4:28 PM, Luis Chamberlain wrote:
> > > On Fri, Jul 09, 2021 at 10:38:12AM -0600, Shuah Khan wrote:
> > > > 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.
> > >
> > > Shuah, curious if you had read this patch from Anirudh Rayabharam
> > > and my response to that v4 patch iteration?
> > >
> > > https://lkml.kernel.org/r/[email protected]
> > >
> >
> > Yes. I realized I am trying to fix the same problem we have been
> > discussing. :) Sorry for the noise.
>
> No worries, and thanks again for you help!
>
> > Ignore my patch. I will follow the thread.
>
> OK ! I think all we need is just Anirudh to split his patch to
> remove the -EAGAIN return value in a separate patch as a first step,
> documenting in the commmit log that:
>
> The only motivation on her part with using -EAGAIN on commit
> 0542ad88fbdd81bb ("firmware loader: Fix _request_firmware_load()
> return val for fw load abort") was to distinguish the error from
> -ENOMEM, and so there is no real reason in keeping it. Keeping
Are you sure about this? As per Shuah's explanation [1], it sounds to
me like certain media drivers explicitly check for -EAGAIN to retry
the firmware load. Shuah, is my understanding correct?
[1]: https://lore.kernel.org/lkml/[email protected]/
> -ETIMEDOUT is much telling of what the reason for a failure is,
> so just use that.
>
> Then his second patch would be simplified without the -EAGAIN
> condition.
>
> All I asked was to confirm that the -ETIMEDOUT was indeed propagated.
Yes, -ETIMEDOUT is indeed propagated by fw_sysfs_wait_timeout.
>
> Anirudh, sorry for the trouble, but can I ask you for a v5 with two
> patches as described above?
Sure, I will do that.
- Anirudh.
>
> Luis