* [PATCH v8 0/2] firmware_loader: fix uaf in firmware_fallback_sysfs @ 2021-07-28 8:51 Anirudh Rayabharam 2021-07-28 8:51 ` [PATCH v8 1/2] firmware_loader: use -ETIMEDOUT instead of -EAGAIN in fw_load_sysfs_fallback Anirudh Rayabharam ` (2 more replies) 0 siblings, 3 replies; 6+ messages in thread From: Anirudh Rayabharam @ 2021-07-28 8:51 UTC (permalink / raw) To: mcgrof, gregkh, rafael, skhan Cc: Anirudh Rayabharam, linux-kernel, linux-kernel-mentees This series fixes the use after free in firmware_fallback_sysfs reported by syzbot at: https://syzkaller.appspot.com/bug?extid=de271708674e2093097b The first patch gets rid of the -EAGAIN return since it doesn't make sense (see patch description for more info). The second patch goes on to actually fix the use after free issue. Changes in v8: 1. Added/fixed some comments as suggested by Shuah Changes in v7: 1. Don't move the error handling code from fw_load_sysfs_fallback to fw_sysfs_wait_timeout to simplify the patch. Also, the move is unnecessary. 2. Fix the commit log for patch 1 as per Luis' suggestions. Changes in v6: 1. v5 didn't actually remove -EAGAIN. So, fixed that. Changes in v5: 1. Split the patch into two patches as discussed here: https://lore.kernel.org/lkml/20210715232105.am4wsxfclj2ufjdw@garbanzo/ Changes in v4: Documented the reasons behind the error codes returned from fw_sysfs_wait_timeout() as suggested by Luis Chamberlain. Changes in v3: Modified the patch to incorporate suggestions by Luis Chamberlain in order to fix the root cause instead of applying a "band-aid" kind of fix. https://lore.kernel.org/lkml/20210403013143.GV4332@42.do-not-panic.com/ Changes in v2: 1. Fixed 1 error and 1 warning (in the commit message) reported by checkpatch.pl. The error was regarding the format for referring to another commit "commit <sha> ("oneline")". The warning was for line longer than 75 chars. Anirudh Rayabharam (2): firmware_loader: use -ETIMEDOUT instead of -EAGAIN in fw_load_sysfs_fallback firmware_loader: fix use-after-free in firmware_fallback_sysfs drivers/base/firmware_loader/fallback.c | 14 ++++++++------ drivers/base/firmware_loader/firmware.h | 10 +++++++++- drivers/base/firmware_loader/main.c | 2 ++ 3 files changed, 19 insertions(+), 7 deletions(-) -- 2.26.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v8 1/2] firmware_loader: use -ETIMEDOUT instead of -EAGAIN in fw_load_sysfs_fallback 2021-07-28 8:51 [PATCH v8 0/2] firmware_loader: fix uaf in firmware_fallback_sysfs Anirudh Rayabharam @ 2021-07-28 8:51 ` Anirudh Rayabharam 2021-07-28 8:51 ` [PATCH v8 2/2] firmware_loader: fix use-after-free in firmware_fallback_sysfs Anirudh Rayabharam 2021-07-28 20:37 ` [PATCH v8 0/2] firmware_loader: fix uaf " Luis Chamberlain 2 siblings, 0 replies; 6+ messages in thread From: Anirudh Rayabharam @ 2021-07-28 8:51 UTC (permalink / raw) To: mcgrof, gregkh, rafael, skhan Cc: Anirudh Rayabharam, linux-kernel, linux-kernel-mentees The only motivation for using -EAGAIN in 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. -EAGAIN is typically used to tell the userspace to try something again and in this case re-using the sysfs loading interface cannot be retried when a timeout happens, so the return value is also bogus. -ETIMEDOUT is received when the wait times out and returning that is much more telling of what the reason for the failure was. So, just propagate that instead of returning -EAGAIN. Suggested-by: Luis Chamberlain <mcgrof@kernel.org> Reviewed-by: Shuah Khan <skhan@linuxfoundation.org> Acked-by: Luis Chamberlain <mcgrof@kernel.org> Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com> --- drivers/base/firmware_loader/fallback.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c index 91899d185e31..1a48be0a030e 100644 --- a/drivers/base/firmware_loader/fallback.c +++ b/drivers/base/firmware_loader/fallback.c @@ -535,8 +535,6 @@ static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs, long timeout) if (fw_state_is_aborted(fw_priv)) { if (retval == -ERESTARTSYS) retval = -EINTR; - else - retval = -EAGAIN; } else if (fw_priv->is_paged_buf && !fw_priv->data) retval = -ENOMEM; -- 2.26.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v8 2/2] firmware_loader: fix use-after-free in firmware_fallback_sysfs 2021-07-28 8:51 [PATCH v8 0/2] firmware_loader: fix uaf in firmware_fallback_sysfs Anirudh Rayabharam 2021-07-28 8:51 ` [PATCH v8 1/2] firmware_loader: use -ETIMEDOUT instead of -EAGAIN in fw_load_sysfs_fallback Anirudh Rayabharam @ 2021-07-28 8:51 ` Anirudh Rayabharam 2021-07-28 16:37 ` Shuah Khan 2021-07-28 20:37 ` [PATCH v8 0/2] firmware_loader: fix uaf " Luis Chamberlain 2 siblings, 1 reply; 6+ messages in thread From: Anirudh Rayabharam @ 2021-07-28 8:51 UTC (permalink / raw) To: mcgrof, gregkh, rafael, skhan Cc: Anirudh Rayabharam, linux-kernel, linux-kernel-mentees, syzbot+de271708674e2093097b This use-after-free happens when a fw_priv object has been freed but hasn't been removed from the pending list (pending_fw_head). The next time fw_load_sysfs_fallback tries to insert into the list, it ends up accessing the pending_list member of the previously freed fw_priv. The root cause here is that all code paths that abort the fw load don't delete it from the pending list. For example: _request_firmware() -> fw_abort_batch_reqs() -> fw_state_aborted() To fix this, delete the fw_priv from the list in __fw_set_state() if the new state is DONE or ABORTED. This way, all aborts will remove the fw_priv from the list. Accordingly, remove calls to list_del_init that were being made before calling fw_state_(aborted|done). Also, in fw_load_sysfs_fallback, don't add the fw_priv to the pending list if it is already aborted. Instead, just jump out and return early. Fixes: bcfbd3523f3c ("firmware: fix a double abort case with fw_load_sysfs_fallback") Reported-by: syzbot+de271708674e2093097b@syzkaller.appspotmail.com Tested-by: syzbot+de271708674e2093097b@syzkaller.appspotmail.com Acked-by: Luis Chamberlain <mcgrof@kernel.org> Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com> --- drivers/base/firmware_loader/fallback.c | 12 ++++++++---- drivers/base/firmware_loader/firmware.h | 10 +++++++++- drivers/base/firmware_loader/main.c | 2 ++ 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c index 1a48be0a030e..d7d63c1aa993 100644 --- a/drivers/base/firmware_loader/fallback.c +++ b/drivers/base/firmware_loader/fallback.c @@ -89,12 +89,11 @@ 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/aborted and disappearance of 'loading' */ - if (fw_sysfs_done(fw_priv)) + if (fw_state_is_aborted(fw_priv) || fw_sysfs_done(fw_priv)) return; - list_del_init(&fw_priv->pending_list); fw_state_aborted(fw_priv); } @@ -280,7 +279,6 @@ static ssize_t firmware_loading_store(struct device *dev, * 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; @@ -513,6 +511,11 @@ static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs, long timeout) } mutex_lock(&fw_lock); + if (fw_state_is_aborted(fw_priv)) { + mutex_unlock(&fw_lock); + retval = -EINTR; + goto out; + } list_add(&fw_priv->pending_list, &pending_fw_head); mutex_unlock(&fw_lock); @@ -538,6 +541,7 @@ static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs, long timeout) } else if (fw_priv->is_paged_buf && !fw_priv->data) retval = -ENOMEM; +out: device_del(f_dev); err_put_dev: put_device(f_dev); diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h index 63bd29fdcb9c..a3014e9e2c85 100644 --- a/drivers/base/firmware_loader/firmware.h +++ b/drivers/base/firmware_loader/firmware.h @@ -117,8 +117,16 @@ static inline void __fw_state_set(struct fw_priv *fw_priv, WRITE_ONCE(fw_st->status, status); - if (status == FW_STATUS_DONE || status == FW_STATUS_ABORTED) + if (status == FW_STATUS_DONE || status == FW_STATUS_ABORTED) { +#ifdef CONFIG_FW_LOADER_USER_HELPER + /* + * Doing this here ensures that the fw_priv is deleted from + * the pending list in all abort/done paths. + */ + list_del_init(&fw_priv->pending_list); +#endif complete_all(&fw_st->completion); + } } static inline void fw_state_aborted(struct fw_priv *fw_priv) diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c index 4fdb8219cd08..68c549d71230 100644 --- a/drivers/base/firmware_loader/main.c +++ b/drivers/base/firmware_loader/main.c @@ -783,8 +783,10 @@ static void fw_abort_batch_reqs(struct firmware *fw) return; fw_priv = fw->priv; + mutex_lock(&fw_lock); if (!fw_state_is_aborted(fw_priv)) fw_state_aborted(fw_priv); + mutex_unlock(&fw_lock); } /* called from request_firmware() and request_firmware_work_func() */ -- 2.26.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v8 2/2] firmware_loader: fix use-after-free in firmware_fallback_sysfs 2021-07-28 8:51 ` [PATCH v8 2/2] firmware_loader: fix use-after-free in firmware_fallback_sysfs Anirudh Rayabharam @ 2021-07-28 16:37 ` Shuah Khan 0 siblings, 0 replies; 6+ messages in thread From: Shuah Khan @ 2021-07-28 16:37 UTC (permalink / raw) To: Anirudh Rayabharam, mcgrof, gregkh, rafael Cc: linux-kernel, linux-kernel-mentees, syzbot+de271708674e2093097b, Shuah Khan On 7/28/21 2:51 AM, Anirudh Rayabharam wrote: > This use-after-free happens when a fw_priv object has been freed but > hasn't been removed from the pending list (pending_fw_head). The next > time fw_load_sysfs_fallback tries to insert into the list, it ends up > accessing the pending_list member of the previously freed fw_priv. > > The root cause here is that all code paths that abort the fw load > don't delete it from the pending list. For example: > > _request_firmware() > -> fw_abort_batch_reqs() > -> fw_state_aborted() > > To fix this, delete the fw_priv from the list in __fw_set_state() if > the new state is DONE or ABORTED. This way, all aborts will remove > the fw_priv from the list. Accordingly, remove calls to list_del_init > that were being made before calling fw_state_(aborted|done). > > Also, in fw_load_sysfs_fallback, don't add the fw_priv to the pending > list if it is already aborted. Instead, just jump out and return early. > > Fixes: bcfbd3523f3c ("firmware: fix a double abort case with fw_load_sysfs_fallback") > Reported-by: syzbot+de271708674e2093097b@syzkaller.appspotmail.com > Tested-by: syzbot+de271708674e2093097b@syzkaller.appspotmail.com > Acked-by: Luis Chamberlain <mcgrof@kernel.org> > Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com> > --- > drivers/base/firmware_loader/fallback.c | 12 ++++++++---- > drivers/base/firmware_loader/firmware.h | 10 +++++++++- > drivers/base/firmware_loader/main.c | 2 ++ > 3 files changed, 19 insertions(+), 5 deletions(-) > Thank you. Looks good. Reviewed-by: Shuah Khan <skhan@linuxfoundation.org> thanks, -- Shuah ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v8 0/2] firmware_loader: fix uaf in firmware_fallback_sysfs 2021-07-28 8:51 [PATCH v8 0/2] firmware_loader: fix uaf in firmware_fallback_sysfs Anirudh Rayabharam 2021-07-28 8:51 ` [PATCH v8 1/2] firmware_loader: use -ETIMEDOUT instead of -EAGAIN in fw_load_sysfs_fallback Anirudh Rayabharam 2021-07-28 8:51 ` [PATCH v8 2/2] firmware_loader: fix use-after-free in firmware_fallback_sysfs Anirudh Rayabharam @ 2021-07-28 20:37 ` Luis Chamberlain 2021-07-29 16:52 ` Anirudh Rayabharam 2 siblings, 1 reply; 6+ messages in thread From: Luis Chamberlain @ 2021-07-28 20:37 UTC (permalink / raw) To: gregkh Cc: rafael, skhan, Anirudh Rayabharam, linux-kernel, linux-kernel-mentees On Wed, Jul 28, 2021 at 02:21:05PM +0530, Anirudh Rayabharam wrote: > This series fixes the use after free in firmware_fallback_sysfs reported by > syzbot at: > https://syzkaller.appspot.com/bug?extid=de271708674e2093097b Greg, With Shua's review ammeded, this series is ready to be queued up, finally. Anirudh, thanks for following up on all these iterations! Luis ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v8 0/2] firmware_loader: fix uaf in firmware_fallback_sysfs 2021-07-28 20:37 ` [PATCH v8 0/2] firmware_loader: fix uaf " Luis Chamberlain @ 2021-07-29 16:52 ` Anirudh Rayabharam 0 siblings, 0 replies; 6+ messages in thread From: Anirudh Rayabharam @ 2021-07-29 16:52 UTC (permalink / raw) To: Luis Chamberlain Cc: gregkh, rafael, skhan, linux-kernel, linux-kernel-mentees On Wed, Jul 28, 2021 at 01:37:09PM -0700, Luis Chamberlain wrote: > On Wed, Jul 28, 2021 at 02:21:05PM +0530, Anirudh Rayabharam wrote: > > This series fixes the use after free in firmware_fallback_sysfs reported by > > syzbot at: > > https://syzkaller.appspot.com/bug?extid=de271708674e2093097b > > Greg, > > With Shua's review ammeded, this series is ready to be queued up, finally. > > Anirudh, thanks for following up on all these iterations! Thank you for your guidance throughout this series! - Anirudh. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-07-29 16:52 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-28 8:51 [PATCH v8 0/2] firmware_loader: fix uaf in firmware_fallback_sysfs Anirudh Rayabharam 2021-07-28 8:51 ` [PATCH v8 1/2] firmware_loader: use -ETIMEDOUT instead of -EAGAIN in fw_load_sysfs_fallback Anirudh Rayabharam 2021-07-28 8:51 ` [PATCH v8 2/2] firmware_loader: fix use-after-free in firmware_fallback_sysfs Anirudh Rayabharam 2021-07-28 16:37 ` Shuah Khan 2021-07-28 20:37 ` [PATCH v8 0/2] firmware_loader: fix uaf " Luis Chamberlain 2021-07-29 16:52 ` 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).