* [PATCH] firmware_loader: Fix use-after-free Read in firmware_loading_store @ 2021-07-08 3:13 Shuah Khan [not found] ` <20210709091721.1869-1-hdanton@sina.com> 0 siblings, 1 reply; 7+ messages in thread From: Shuah Khan @ 2021-07-08 3:13 UTC (permalink / raw) To: mcgrof, gregkh, rafael Cc: Shuah Khan, linux-kernel, syzbot+77cea49e091776a57689 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@syzkaller.appspotmail.com Signed-off-by: Shuah Khan <skhan@linuxfoundation.org> --- 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
[parent not found: <20210709091721.1869-1-hdanton@sina.com>]
* Re: [PATCH] firmware_loader: Fix use-after-free Read in firmware_loading_store [not found] ` <20210709091721.1869-1-hdanton@sina.com> @ 2021-07-09 16:15 ` Shuah Khan 2021-07-09 16:38 ` Shuah Khan 0 siblings, 1 reply; 7+ messages in thread From: Shuah Khan @ 2021-07-09 16:15 UTC (permalink / raw) To: Hillf Danton Cc: mcgrof, gregkh, rafael, linux-kernel, syzbot+77cea49e091776a57689, Shuah Khan 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@syzkaller.appspotmail.com >> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org> >> --- >> 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] firmware_loader: Fix use-after-free Read in firmware_loading_store 2021-07-09 16:15 ` Shuah Khan @ 2021-07-09 16:38 ` Shuah Khan 2021-07-15 22:28 ` Luis Chamberlain 0 siblings, 1 reply; 7+ messages in thread From: Shuah Khan @ 2021-07-09 16:38 UTC (permalink / raw) To: Hillf Danton Cc: mcgrof, gregkh, rafael, linux-kernel, syzbot+77cea49e091776a57689, Shuah Khan 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@syzkaller.appspotmail.com >>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org> >>> --- >>> 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] firmware_loader: Fix use-after-free Read in firmware_loading_store 2021-07-09 16:38 ` Shuah Khan @ 2021-07-15 22:28 ` Luis Chamberlain 2021-07-15 22:46 ` Shuah Khan 0 siblings, 1 reply; 7+ messages in thread From: Luis Chamberlain @ 2021-07-15 22:28 UTC (permalink / raw) To: Shuah Khan, Anirudh Rayabharam Cc: Hillf Danton, gregkh, rafael, linux-kernel, syzbot+77cea49e091776a57689 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/20210518155921.4181-1-mail@anirudhrb.com Luis ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] firmware_loader: Fix use-after-free Read in firmware_loading_store 2021-07-15 22:28 ` Luis Chamberlain @ 2021-07-15 22:46 ` Shuah Khan 2021-07-15 23:21 ` Luis Chamberlain 0 siblings, 1 reply; 7+ messages in thread From: Shuah Khan @ 2021-07-15 22:46 UTC (permalink / raw) To: Luis Chamberlain, Anirudh Rayabharam Cc: Hillf Danton, gregkh, rafael, linux-kernel, syzbot+77cea49e091776a57689, Shuah Khan 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/20210518155921.4181-1-mail@anirudhrb.com > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] firmware_loader: Fix use-after-free Read in firmware_loading_store 2021-07-15 22:46 ` Shuah Khan @ 2021-07-15 23:21 ` Luis Chamberlain 2021-07-18 9:01 ` Anirudh Rayabharam 0 siblings, 1 reply; 7+ messages in thread From: Luis Chamberlain @ 2021-07-15 23:21 UTC (permalink / raw) To: Shuah Khan Cc: Anirudh Rayabharam, Hillf Danton, gregkh, rafael, linux-kernel, syzbot+77cea49e091776a57689 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/20210518155921.4181-1-mail@anirudhrb.com > > > > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] firmware_loader: Fix use-after-free Read in firmware_loading_store 2021-07-15 23:21 ` Luis Chamberlain @ 2021-07-18 9:01 ` Anirudh Rayabharam 0 siblings, 0 replies; 7+ messages in thread From: Anirudh Rayabharam @ 2021-07-18 9:01 UTC (permalink / raw) To: Luis Chamberlain Cc: Shuah Khan, Hillf Danton, gregkh, rafael, linux-kernel, syzbot+77cea49e091776a57689, mail 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/20210518155921.4181-1-mail@anirudhrb.com > > > > > > > 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/04b5bb2f-edf7-5b43-585a-3267d83bd8c3@linuxfoundation.org/ > -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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-07-18 9:02 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-08 3:13 [PATCH] firmware_loader: Fix use-after-free Read in firmware_loading_store Shuah Khan [not found] ` <20210709091721.1869-1-hdanton@sina.com> 2021-07-09 16:15 ` Shuah Khan 2021-07-09 16:38 ` Shuah Khan 2021-07-15 22:28 ` Luis Chamberlain 2021-07-15 22:46 ` Shuah Khan 2021-07-15 23:21 ` Luis Chamberlain 2021-07-18 9:01 ` Anirudh Rayabharam
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).