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/[email protected]/
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
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 <[email protected]>
Reviewed-by: Shuah Khan <[email protected]>
Acked-by: Luis Chamberlain <[email protected]>
Signed-off-by: Anirudh Rayabharam <[email protected]>
---
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
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: [email protected]
Tested-by: [email protected]
Acked-by: Luis Chamberlain <[email protected]>
Signed-off-by: Anirudh Rayabharam <[email protected]>
---
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
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: [email protected]
> Tested-by: [email protected]
> Acked-by: Luis Chamberlain <[email protected]>
> Signed-off-by: Anirudh Rayabharam <[email protected]>
> ---
> 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 <[email protected]>
thanks,
-- Shuah
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
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.