[PATCH v2] firmware_loader: fix use-after-free in firmware_fallback_sysfs

[PATCH v2] firmware_loader: fix use-after-free in firmware_fallback_sysfs

[Anirudh Rayabharam]

The 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.

In commit bcfbd3523f3c ("firmware: fix a double abort case with
fw_load_sysfs_fallback"), fw_load_abort() is skipped if
fw_sysfs_wait_timeout() returns -ENOENT. This causes the fw_priv to
not be removed from the pending list.

To fix this, delete the fw_priv from the pending list when retval
is -ENOENT instead of skipping the entire block.

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 <mail@anirudhrb.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.

---
 drivers/base/firmware_loader/fallback.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
index 91899d185e31..56ae4ab3199d 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -526,9 +526,14 @@ 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);
+
+		if (retval != -ENOENT)
+			fw_load_abort(fw_sysfs);
+		else
+			list_del_init(&fw_priv->pending_list);
+
 		mutex_unlock(&fw_lock);
 	}
 
-- 
2.26.2

Re: [PATCH v2] firmware_loader: fix use-after-free in firmware_fallback_sysfs

[Luis Chamberlain]

On Tue, Apr 13, 2021 at 04:12:42PM +0530, Anirudh Rayabharam wrote:
> The 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.
> 
> In commit bcfbd3523f3c ("firmware: fix a double abort case with
> fw_load_sysfs_fallback"), fw_load_abort() is skipped if
> fw_sysfs_wait_timeout() returns -ENOENT. This causes the fw_priv to
> not be removed from the pending list.
> 
> To fix this, delete the fw_priv from the pending list when retval
> is -ENOENT instead of skipping the entire block.
> 
> 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 <mail@anirudhrb.com>

Thanks for your patch Anirudh, but please also see this reply to the
issue:

http://lkml.kernel.org/r/20210403013143.GV4332@42.do-not-panic.com

The way you patched the issue is just a band-aid, meaning we keep on
moving the issue further and it seems that's just the wrong approach.

Can you try the patch in that thread, to verify if the UAF goes away?

Re: [PATCH v2] firmware_loader: fix use-after-free in firmware_fallback_sysfs

[Anirudh Rayabharam]

On Tue, Apr 13, 2021 at 04:51:38PM +0000, Luis Chamberlain wrote:
> On Tue, Apr 13, 2021 at 04:12:42PM +0530, Anirudh Rayabharam wrote:
> > The 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.
> > 
> > In commit bcfbd3523f3c ("firmware: fix a double abort case with
> > fw_load_sysfs_fallback"), fw_load_abort() is skipped if
> > fw_sysfs_wait_timeout() returns -ENOENT. This causes the fw_priv to
> > not be removed from the pending list.
> > 
> > To fix this, delete the fw_priv from the pending list when retval
> > is -ENOENT instead of skipping the entire block.
> > 
> > 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 <mail@anirudhrb.com>
> 
> Thanks for your patch Anirudh, but please also see this reply to the
> issue:
> 
> http://lkml.kernel.org/r/20210403013143.GV4332@42.do-not-panic.com

Hi Luis! Thanks for pointing me to this. I completely forgot to check
the existing discussion on this issue.

> 
> The way you patched the issue is just a band-aid, meaning we keep on
> moving the issue further and it seems that's just the wrong approach.
> 
> Can you try the patch in that thread, to verify if the UAF goes away?

The patch in that thread doesn't work. But I think I know what's
missing. The root problem here is that all code paths that abort fw load
don't remove it from the pending list. For example:

	_request_firmware()
	 -> fw_abort_batch_reqs()
	   -> fw_state_aborted()

In the above code path, the fw_priv is aborted but not removed from
pending list. So, the patch in the above thread fails because the load
is being aborted after it has been added to the list.

So, to fix the root cause of this issue we should make it so that all
aborts remove the fw_priv from the pending list. Perhaps we should add
a list_del_init in __fw_set_state() just before calling complete_all().
This way, all code paths that abort will delete the fw_priv from the
list.

The patch in the above thread also makes some changes to the error
codes. Since it isn't directly related to the UAF, the error codes
change should be a separate patch right?

Thanks!

	- Anirudh.

Re: [PATCH v2] firmware_loader: fix use-after-free in firmware_fallback_sysfs

[Anirudh Rayabharam]

On Tue, Apr 13, 2021 at 04:51:38PM +0000, Luis Chamberlain wrote:
> On Tue, Apr 13, 2021 at 04:12:42PM +0530, Anirudh Rayabharam wrote:
> > The 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.
> > 
> > In commit bcfbd3523f3c ("firmware: fix a double abort case with
> > fw_load_sysfs_fallback"), fw_load_abort() is skipped if
> > fw_sysfs_wait_timeout() returns -ENOENT. This causes the fw_priv to
> > not be removed from the pending list.
> > 
> > To fix this, delete the fw_priv from the pending list when retval
> > is -ENOENT instead of skipping the entire block.
> > 
> > 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 <mail@anirudhrb.com>
> 
> Thanks for your patch Anirudh, but please also see this reply to the
> issue:
> 
> http://lkml.kernel.org/r/20210403013143.GV4332@42.do-not-panic.com

I have now sent a v3 that is more along the lines of the patch suggested
in the above thread.

Thanks!

	- Anirudh.