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 does some cleanup of the error codes and documents
them properly. The second patch goes on to actually fix the bug.
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 | 44 +++++++++++++++++--------
drivers/base/firmware_loader/firmware.h | 6 +++-
drivers/base/firmware_loader/main.c | 2 ++
3 files changed, 38 insertions(+), 14 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. Keeping -ETIMEDOU is much telling of what the
reason for a failure is, so just use that.
The rest is just trying to document a bit more of the motivations for the
error codes, as otherwise we'd lose this information easily.
Suggested-by: Luis Chamberlain <[email protected]>
Signed-off-by: Anirudh Rayabharam <[email protected]>
---
drivers/base/firmware_loader/fallback.c | 34 +++++++++++++++++--------
1 file changed, 24 insertions(+), 10 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
index 91899d185e31..1db94165feaf 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -70,7 +70,29 @@ static inline bool fw_sysfs_loading(struct fw_priv *fw_priv)
static inline int fw_sysfs_wait_timeout(struct fw_priv *fw_priv, long timeout)
{
- return __fw_state_wait_common(fw_priv, timeout);
+ int ret = __fw_state_wait_common(fw_priv, timeout);
+
+ /*
+ * A signal could be sent to abort a wait. Consider Android's init
+ * gettting a SIGCHLD, which in turn was the same process issuing the
+ * sysfs store call for the fallback. In such cases we want to be able
+ * to tell apart in userspace when a signal caused a failure on the
+ * wait. In such cases we'd get -ERESTARTSYS.
+ *
+ * Likewise though another race can happen and abort the load earlier.
+ *
+ * In either case the situation is interrupted so we just inform
+ * userspace of that and we end things right away.
+ *
+ * When we really time out just tell userspace it should try again,
+ * perhaps later.
+ */
+ if (ret == -ERESTARTSYS || fw_state_is_aborted(fw_priv))
+ ret = -EINTR;
+ else if (fw_priv->is_paged_buf && !fw_priv->data)
+ ret = -ENOMEM;
+
+ return ret;
}
struct fw_sysfs {
@@ -526,20 +548,12 @@ static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs, long timeout)
}
retval = fw_sysfs_wait_timeout(fw_priv, timeout);
- if (retval < 0 && retval != -ENOENT) {
+ if (retval < 0) {
mutex_lock(&fw_lock);
fw_load_abort(fw_sysfs);
mutex_unlock(&fw_lock);
}
- 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;
-
device_del(f_dev);
err_put_dev:
put_device(f_dev);
--
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 1db94165feaf..d262332374eb 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -113,10 +113,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);
}
@@ -302,7 +301,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;
@@ -535,6 +533,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);
@@ -554,6 +557,7 @@ static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs, long timeout)
mutex_unlock(&fw_lock);
}
+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
On Thu, Jul 22, 2021 at 06:02:28PM +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. Keeping -ETIMEDOU is much telling of what the
Since you'll have to respin, a missing here ^, also add that the
-ETIMEDOUT is what we'd get when we do time out on the wait, as its
not clear from the conext being changed.
> reason for a failure is, so just use that.
>
> The rest is just trying to document a bit more of the motivations for the
> error codes, as otherwise we'd lose this information easily.
This is a separate change, and it actually does more than just that.
Moving code around should be done separately. The idea is to
first just remove the -EAGAIN so that the change is *easy* to review.
A remove of a return code *and* a move of code around makes it less
obvious for code review. And part of the comment is wrong now that we
removed -EAGAIN. When breaking patches up please review each change
going into each patch and consider if it makes sense, atomically.
> Suggested-by: Luis Chamberlain <[email protected]>
> Signed-off-by: Anirudh Rayabharam <[email protected]>
> ---
> drivers/base/firmware_loader/fallback.c | 34 +++++++++++++++++--------
> 1 file changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
> index 91899d185e31..1db94165feaf 100644
> --- a/drivers/base/firmware_loader/fallback.c
> +++ b/drivers/base/firmware_loader/fallback.c
> @@ -70,7 +70,29 @@ static inline bool fw_sysfs_loading(struct fw_priv *fw_priv)
>
> static inline int fw_sysfs_wait_timeout(struct fw_priv *fw_priv, long timeout)
> {
> - return __fw_state_wait_common(fw_priv, timeout);
> + int ret = __fw_state_wait_common(fw_priv, timeout);
> +
> + /*
> + * A signal could be sent to abort a wait. Consider Android's init
> + * gettting a SIGCHLD, which in turn was the same process issuing the
> + * sysfs store call for the fallback. In such cases we want to be able
> + * to tell apart in userspace when a signal caused a failure on the
> + * wait. In such cases we'd get -ERESTARTSYS.
> + *
> + * Likewise though another race can happen and abort the load earlier.
This comment is about the check for fw_load_abort() so since the move is
not going to happen when you remove -EAGAIN just leave it out. It can be
added once you do the move.
> + *
> + * In either case the situation is interrupted so we just inform
> + * userspace of that and we end things right away.
Be mindful that this is in context of both cases when re-writing the
patches.
> + *
> + * When we really time out just tell userspace it should try again,
> + * perhaps later.
That's the thing, we're getting rid of that -EAGAIN as it made no sense,
the goal was to just distinguish the error from -ENOMEM. That's it.
Since we are removing the -EAGAIN, this comment makes no sense as we
have clarified with Shuah that the goal of her patch was just to
distinguish the error.
So "tell userspace to try again" makes no sense since if a timeout
happened userspace can't really try again as we have aborted the whole
operation to allow firmware to be uploaded.
In fact, please add that to the commit log which removes the -EAGAIN,
something like:
"Using -EAGAIN is also not correct as this return code is typically used
to tell userspace to try something again, in this case re-using the
sysfs loading interface cannot be retried when a timeout happens, so
the return value is also bogus."
> + */
> + if (ret == -ERESTARTSYS || fw_state_is_aborted(fw_priv))
> + ret = -EINTR;
> + else if (fw_priv->is_paged_buf && !fw_priv->data)
> + ret = -ENOMEM;
> +
> + return ret;
> }
>
> struct fw_sysfs {
> @@ -526,20 +548,12 @@ static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs, long timeout)
> }
>
> retval = fw_sysfs_wait_timeout(fw_priv, timeout);
> - if (retval < 0 && retval != -ENOENT) {
> + if (retval < 0) {
> mutex_lock(&fw_lock);
> fw_load_abort(fw_sysfs);
> mutex_unlock(&fw_lock);
> }
>
> - if (fw_state_is_aborted(fw_priv)) {
> - if (retval == -ERESTARTSYS)
> - retval = -EINTR;
> - else
> - retval = -EAGAIN;
All we want to do is remove this -EAGAIN line in one patch. We
don't want to move code to another place. We do this to make code
easier to review.
We preserve the error code from the wait when a signal did not interrupt
the process (-ERESTARTSYS), and so this can only be -ETIMEDOUT.
> - } else if (fw_priv->is_paged_buf && !fw_priv->data)
> - retval = -ENOMEM;
> -
Thanks for keeping up with the series!
Luis
On Thu, Jul 22, 2021 at 12:59:24PM -0700, Luis Chamberlain wrote:
> On Thu, Jul 22, 2021 at 06:02:28PM +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. Keeping -ETIMEDOU is much telling of what the
>
> Since you'll have to respin, a missing here ^, also add that the
> -ETIMEDOUT is what we'd get when we do time out on the wait, as its
> not clear from the conext being changed.
>
> > reason for a failure is, so just use that.
> >
> > The rest is just trying to document a bit more of the motivations for the
> > error codes, as otherwise we'd lose this information easily.
>
> This is a separate change, and it actually does more than just that.
> Moving code around should be done separately. The idea is to
> first just remove the -EAGAIN so that the change is *easy* to review.
> A remove of a return code *and* a move of code around makes it less
> obvious for code review. And part of the comment is wrong now that we
> removed -EAGAIN. When breaking patches up please review each change
> going into each patch and consider if it makes sense, atomically.
>
> > Suggested-by: Luis Chamberlain <[email protected]>
> > Signed-off-by: Anirudh Rayabharam <[email protected]>
> > ---
> > drivers/base/firmware_loader/fallback.c | 34 +++++++++++++++++--------
> > 1 file changed, 24 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
> > index 91899d185e31..1db94165feaf 100644
> > --- a/drivers/base/firmware_loader/fallback.c
> > +++ b/drivers/base/firmware_loader/fallback.c
> > @@ -70,7 +70,29 @@ static inline bool fw_sysfs_loading(struct fw_priv *fw_priv)
> >
> > static inline int fw_sysfs_wait_timeout(struct fw_priv *fw_priv, long timeout)
> > {
> > - return __fw_state_wait_common(fw_priv, timeout);
> > + int ret = __fw_state_wait_common(fw_priv, timeout);
> > +
> > + /*
> > + * A signal could be sent to abort a wait. Consider Android's init
> > + * gettting a SIGCHLD, which in turn was the same process issuing the
> > + * sysfs store call for the fallback. In such cases we want to be able
> > + * to tell apart in userspace when a signal caused a failure on the
> > + * wait. In such cases we'd get -ERESTARTSYS.
> > + *
> > + * Likewise though another race can happen and abort the load earlier.
>
> This comment is about the check for fw_load_abort() so since the move is
> not going to happen when you remove -EAGAIN just leave it out. It can be
> added once you do the move.
>
> > + *
> > + * In either case the situation is interrupted so we just inform
> > + * userspace of that and we end things right away.
>
> Be mindful that this is in context of both cases when re-writing the
> patches.
>
> > + *
> > + * When we really time out just tell userspace it should try again,
> > + * perhaps later.
>
> That's the thing, we're getting rid of that -EAGAIN as it made no sense,
> the goal was to just distinguish the error from -ENOMEM. That's it.
> Since we are removing the -EAGAIN, this comment makes no sense as we
> have clarified with Shuah that the goal of her patch was just to
> distinguish the error.
>
> So "tell userspace to try again" makes no sense since if a timeout
> happened userspace can't really try again as we have aborted the whole
> operation to allow firmware to be uploaded.
>
> In fact, please add that to the commit log which removes the -EAGAIN,
> something like:
>
> "Using -EAGAIN is also not correct as this return code is typically used
> to tell userspace to try something again, in this case re-using the
> sysfs loading interface cannot be retried when a timeout happens, so
> the return value is also bogus."
>
> > + */
> > + if (ret == -ERESTARTSYS || fw_state_is_aborted(fw_priv))
> > + ret = -EINTR;
> > + else if (fw_priv->is_paged_buf && !fw_priv->data)
> > + ret = -ENOMEM;
> > +
> > + return ret;
> > }
> >
> > struct fw_sysfs {
> > @@ -526,20 +548,12 @@ static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs, long timeout)
> > }
> >
> > retval = fw_sysfs_wait_timeout(fw_priv, timeout);
> > - if (retval < 0 && retval != -ENOENT) {
> > + if (retval < 0) {
> > mutex_lock(&fw_lock);
> > fw_load_abort(fw_sysfs);
> > mutex_unlock(&fw_lock);
> > }
> >
> > - if (fw_state_is_aborted(fw_priv)) {
> > - if (retval == -ERESTARTSYS)
> > - retval = -EINTR;
> > - else
> > - retval = -EAGAIN;
>
> All we want to do is remove this -EAGAIN line in one patch. We
> don't want to move code to another place. We do this to make code
Is the move necessary or should I drop it from this series entirely?
Thanks for the review!
- Anirudh.
> easier to review.
>
> We preserve the error code from the wait when a signal did not interrupt
> the process (-ERESTARTSYS), and so this can only be -ETIMEDOUT.
>
> > - } else if (fw_priv->is_paged_buf && !fw_priv->data)
> > - retval = -ENOMEM;
> > -
>
> Thanks for keeping up with the series!
>
> Luis
On Fri, Jul 23, 2021 at 07:28:59PM +0530, Anirudh Rayabharam wrote:
> On Thu, Jul 22, 2021 at 12:59:24PM -0700, Luis Chamberlain wrote:
> > On Thu, Jul 22, 2021 at 06:02:28PM +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. Keeping -ETIMEDOU is much telling of what the
> >
> > Since you'll have to respin, a missing here ^, also add that the
> > -ETIMEDOUT is what we'd get when we do time out on the wait, as its
> > not clear from the conext being changed.
> >
> > > reason for a failure is, so just use that.
> > >
> > > The rest is just trying to document a bit more of the motivations for the
> > > error codes, as otherwise we'd lose this information easily.
> >
> > This is a separate change, and it actually does more than just that.
> > Moving code around should be done separately. The idea is to
> > first just remove the -EAGAIN so that the change is *easy* to review.
> > A remove of a return code *and* a move of code around makes it less
> > obvious for code review. And part of the comment is wrong now that we
> > removed -EAGAIN. When breaking patches up please review each change
> > going into each patch and consider if it makes sense, atomically.
> >
> > > Suggested-by: Luis Chamberlain <[email protected]>
> > > Signed-off-by: Anirudh Rayabharam <[email protected]>
> > > ---
> > > drivers/base/firmware_loader/fallback.c | 34 +++++++++++++++++--------
> > > 1 file changed, 24 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
> > > index 91899d185e31..1db94165feaf 100644
> > > --- a/drivers/base/firmware_loader/fallback.c
> > > +++ b/drivers/base/firmware_loader/fallback.c
> > > @@ -70,7 +70,29 @@ static inline bool fw_sysfs_loading(struct fw_priv *fw_priv)
> > >
> > > static inline int fw_sysfs_wait_timeout(struct fw_priv *fw_priv, long timeout)
> > > {
> > > - return __fw_state_wait_common(fw_priv, timeout);
> > > + int ret = __fw_state_wait_common(fw_priv, timeout);
> > > +
> > > + /*
> > > + * A signal could be sent to abort a wait. Consider Android's init
> > > + * gettting a SIGCHLD, which in turn was the same process issuing the
> > > + * sysfs store call for the fallback. In such cases we want to be able
> > > + * to tell apart in userspace when a signal caused a failure on the
> > > + * wait. In such cases we'd get -ERESTARTSYS.
> > > + *
> > > + * Likewise though another race can happen and abort the load earlier.
> >
> > This comment is about the check for fw_load_abort() so since the move is
> > not going to happen when you remove -EAGAIN just leave it out. It can be
> > added once you do the move.
> >
> > > + *
> > > + * In either case the situation is interrupted so we just inform
> > > + * userspace of that and we end things right away.
> >
> > Be mindful that this is in context of both cases when re-writing the
> > patches.
> >
> > > + *
> > > + * When we really time out just tell userspace it should try again,
> > > + * perhaps later.
> >
> > That's the thing, we're getting rid of that -EAGAIN as it made no sense,
> > the goal was to just distinguish the error from -ENOMEM. That's it.
> > Since we are removing the -EAGAIN, this comment makes no sense as we
> > have clarified with Shuah that the goal of her patch was just to
> > distinguish the error.
> >
> > So "tell userspace to try again" makes no sense since if a timeout
> > happened userspace can't really try again as we have aborted the whole
> > operation to allow firmware to be uploaded.
> >
> > In fact, please add that to the commit log which removes the -EAGAIN,
> > something like:
> >
> > "Using -EAGAIN is also not correct as this return code is typically used
> > to tell userspace to try something again, in this case re-using the
> > sysfs loading interface cannot be retried when a timeout happens, so
> > the return value is also bogus."
> >
> > > + */
> > > + if (ret == -ERESTARTSYS || fw_state_is_aborted(fw_priv))
> > > + ret = -EINTR;
> > > + else if (fw_priv->is_paged_buf && !fw_priv->data)
> > > + ret = -ENOMEM;
> > > +
> > > + return ret;
> > > }
> > >
> > > struct fw_sysfs {
> > > @@ -526,20 +548,12 @@ static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs, long timeout)
> > > }
> > >
> > > retval = fw_sysfs_wait_timeout(fw_priv, timeout);
> > > - if (retval < 0 && retval != -ENOENT) {
> > > + if (retval < 0) {
> > > mutex_lock(&fw_lock);
> > > fw_load_abort(fw_sysfs);
> > > mutex_unlock(&fw_lock);
> > > }
> > >
> > > - if (fw_state_is_aborted(fw_priv)) {
> > > - if (retval == -ERESTARTSYS)
> > > - retval = -EINTR;
> > > - else
> > > - retval = -EAGAIN;
> >
> > All we want to do is remove this -EAGAIN line in one patch. We
> > don't want to move code to another place. We do this to make code
>
> Is the move necessary or should I drop it from this series entirely?
The move is possible, sure. Maybe do that in a separate patch. But
just read each patch as you write it, and make sure they do just *one*
thing at a time. Re-read the patch once done and make sure each
patch makes sense on its own.
Luis