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/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 | 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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
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: syzbot+de271708674e2093097b@syzkaller.appspotmail.com
Tested-by: syzbot+de271708674e2093097b@syzkaller.appspotmail.com
Signed-off-by: Anirudh Rayabharam <redacted>
---
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(-)
@@ -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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
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: syzbot+de271708674e2093097b@syzkaller.appspotmail.com
Tested-by: syzbot+de271708674e2093097b@syzkaller.appspotmail.com
Signed-off-by: Anirudh Rayabharam <redacted>
---
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(-)
Update the comment to "between loading done/abort"
Rest looks good to me.
quoted hunk
- 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);
@@ -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() */
From: Luis Chamberlain <mcgrof@kernel.org> Date: 2021-07-26 18:27:29
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: syzbot+de271708674e2093097b@syzkaller.appspotmail.com
Tested-by: syzbot+de271708674e2093097b@syzkaller.appspotmail.com
Curious, how do you get syzbot to test this, I mean your custom tree?
Signed-off-by: Anirudh Rayabharam <redacted>
With the changes Shua requested being made:
Acked-by: Luis Chamberlain <mcgrof@kernel.org>
Luis
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
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:
quoted
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: syzbot+de271708674e2093097b@syzkaller.appspotmail.com
Tested-by: syzbot+de271708674e2093097b@syzkaller.appspotmail.com
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.
quoted
Signed-off-by: Anirudh Rayabharam <redacted>
With the changes Shua requested being made:
I'll implement Shua's suggestions and send a new version.
Thanks!
- Anirudh.
Acked-by: Luis Chamberlain <mcgrof@kernel.org>
Luis
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>
Signed-off-by: Anirudh Rayabharam <redacted>
---
drivers/base/firmware_loader/fallback.c | 2 --
1 file changed, 2 deletions(-)
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>
Signed-off-by: Anirudh Rayabharam <redacted>
---
drivers/base/firmware_loader/fallback.c | 2 --
1 file changed, 2 deletions(-)
From: Luis Chamberlain <mcgrof@kernel.org> Date: 2021-07-26 18:07:57
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 <mcgrof@kernel.org>
Signed-off-by: Anirudh Rayabharam <redacted>
Acked-by: Luis Chamberlain <mcgrof@kernel.org>
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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees