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 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 the 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 | 12 +++++++-----
drivers/base/firmware_loader/firmware.h | 6 +++++-
drivers/base/firmware_loader/main.c | 2 ++
3 files changed, 14 insertions(+), 6 deletions(-)
--
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 previoiusly 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]
Signed-off-by: Anirudh Rayabharam <[email protected]>
---
drivers/base/firmware_loader/fallback.c | 10 +++++++---
drivers/base/firmware_loader/firmware.h | 6 +++++-
drivers/base/firmware_loader/main.c | 2 ++
3 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
index 1a48be0a030e..5d9f372fc9e7 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -91,10 +91,9 @@ 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'
*/
- 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..36bdb413c998 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -117,8 +117,12 @@ 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
+ 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
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]>
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
On 7/24/21 6:11 AM, Anirudh Rayabharam wrote:
> 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]>
> 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;
>
>
Reviewed-by: Shuah Khan <[email protected]>
thanks,
-- Shuah
On 7/24/21 6:11 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 previoiusly 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]
> Signed-off-by: Anirudh Rayabharam <[email protected]>
> ---
> drivers/base/firmware_loader/fallback.c | 10 +++++++---
> drivers/base/firmware_loader/firmware.h | 6 +++++-
> drivers/base/firmware_loader/main.c | 2 ++
> 3 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
> index 1a48be0a030e..5d9f372fc9e7 100644
> --- a/drivers/base/firmware_loader/fallback.c
> +++ b/drivers/base/firmware_loader/fallback.c
> @@ -91,10 +91,9 @@ 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'
> */
Update the comment to "between loading done/abort"
Rest looks good to me.
> - 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..36bdb413c998 100644
> --- a/drivers/base/firmware_loader/firmware.h
> +++ b/drivers/base/firmware_loader/firmware.h
> @@ -117,8 +117,12 @@ 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
Might be helpful to add a comment that this covers 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() */
>
thanks,
-- Shuah
On Sat, Jul 24, 2021 at 05:41:33PM +0530, Anirudh Rayabharam wrote:
> 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]>
> Signed-off-by: Anirudh Rayabharam <[email protected]>
Acked-by: Luis Chamberlain <[email protected]>
Beautiful.
As you see, without any of this documented in a commit log, the change
would have been very obscure. Now we cleary document a userspace
changing return value clearly.
Thanks!
Luis
On Sat, Jul 24, 2021 at 05:41:34PM +0530, 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 previoiusly 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]
Curious, how do you get syzbot to test this, I mean your custom tree?
> Signed-off-by: Anirudh Rayabharam <[email protected]>
With the changes Shua requested being made:
Acked-by: Luis Chamberlain <[email protected]>
Luis
On Mon, Jul 26, 2021 at 11:27:21AM -0700, Luis Chamberlain wrote:
> On Sat, Jul 24, 2021 at 05:41:34PM +0530, 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 previoiusly 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]
>
> Curious, how do you get syzbot to test this, I mean your custom tree?
Don't need a custom tree. You can send syzbot the git url of an existing
tree (such as linux-next or Linus' tree) and a patch to apply on that
tree before testing. This is documented here:
https://github.com/google/syzkaller/blob/master/docs/syzbot.md#testing-patches
Of course, using a custom tree is also possible.
>
> > Signed-off-by: Anirudh Rayabharam <[email protected]>
>
> With the changes Shua requested being made:
I'll implement Shua's suggestions and send a new version.
Thanks!
- Anirudh.
>
> Acked-by: Luis Chamberlain <[email protected]>
>
> Luis