[PATCH] drivers/firmware_loader: remove list entry before deallocation

[PATCH] drivers/firmware_loader: remove list entry before deallocation

[Michal Lach]

If CONFIG_FW_LOADER_USER_HELPER is enabled, it is possible to interrupt
the loading process after adding pending_list to pending_fw_list.
Subsequently, if user calls release_firmware() which deallocates the
fw_priv structure which pending_list is a member of, the entry in the
list is not deleted. This causes a use-after-free on further attempts
to add an entry to the list or on list traversal.

While not problematic in most drivers since this function is mainly used
in probe or init routines, some drivers expose firmware loading
functionality via user-accessible functions like write() etc.
In this case during the sysfs loading process, we can send SIGKILL to the
thread which is then in kernel, leave the entry in the list and then free
the structure.

Example kernel panics with CONFIG_DEBUG_LIST turned on:

kernel BUG at lib/list_debug.c:25!
/* ... */
Call trace:
 __list_add_valid+0x7c/0x98
 fw_load_sysfs_fallback+0xd4/0x334
 fw_load_from_user_helper+0x148/0x1f8
 firmware_fallback_sysfs+0xe0/0x17c
 _request_firmware+0x1a0/0x470
 request_firmware+0x50/0x78
/* ... */

or

kernel BUG at lib/list_debug.c:56!
/* ... */
Call trace:
 __list_del_entry_valid+0xa0/0xa4
 fw_load_abort+0x38/0x64
 fw_load_sysfs_fallback+0x354/0x468
 fw_load_from_user_helper+0x17c/0x1c0
 firmware_fallback_sysfs+0xc0/0x11c
 _request_firmware+0xe0/0x4a4
 request_firmware+0x20/0x2c
/* ... */

Fixes: fe304143b0c3 ("firmware: Avoid deadlock of usermodehelper lock at shutdown")
Signed-off-by: Michal Lach <michal.lach@samsung.com>
---
 drivers/base/firmware_loader/main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index 7c3590fd97c2..381997c84e4f 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -209,6 +209,10 @@ static void __free_fw_priv(struct kref *ref)
 		 __func__, fw_priv->fw_name, fw_priv, fw_priv->data,
 		 (unsigned int)fw_priv->size);
 
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+	list_del(&fw_priv->pending_list);
+#endif
+
 	list_del(&fw_priv->list);
 	spin_unlock(&fwc->lock);
 
-- 
2.25.1

Re: [PATCH] drivers/firmware_loader: remove list entry before deallocation

[Russ Weight]

On 11/23/22 03:14, Michal Lach wrote:
> If CONFIG_FW_LOADER_USER_HELPER is enabled, it is possible to interrupt
> the loading process after adding pending_list to pending_fw_list.
> Subsequently, if user calls release_firmware() which deallocates the
> fw_priv structure which pending_list is a member of, the entry in the
> list is not deleted. This causes a use-after-free on further attempts
> to add an entry to the list or on list traversal.
>
> While not problematic in most drivers since this function is mainly used
> in probe or init routines, some drivers expose firmware loading
> functionality via user-accessible functions like write() etc.
> In this case during the sysfs loading process, we can send SIGKILL to the
> thread which is then in kernel, leave the entry in the list and then free
> the structure.
>
> Example kernel panics with CONFIG_DEBUG_LIST turned on:
>
> kernel BUG at lib/list_debug.c:25!
> /* ... */
> Call trace:
>  __list_add_valid+0x7c/0x98
>  fw_load_sysfs_fallback+0xd4/0x334
>  fw_load_from_user_helper+0x148/0x1f8
>  firmware_fallback_sysfs+0xe0/0x17c
>  _request_firmware+0x1a0/0x470
>  request_firmware+0x50/0x78
> /* ... */
>
> or
>
> kernel BUG at lib/list_debug.c:56!
> /* ... */
> Call trace:
>  __list_del_entry_valid+0xa0/0xa4
>  fw_load_abort+0x38/0x64
>  fw_load_sysfs_fallback+0x354/0x468
>  fw_load_from_user_helper+0x17c/0x1c0
>  firmware_fallback_sysfs+0xc0/0x11c
>  _request_firmware+0xe0/0x4a4
>  request_firmware+0x20/0x2c
> /* ... */
>
> Fixes: fe304143b0c3 ("firmware: Avoid deadlock of usermodehelper lock at shutdown")

Reviewed-by: Russ Weight <russell.h.weight@intel.com>

> Signed-off-by: Michal Lach <michal.lach@samsung.com>
> ---
>  drivers/base/firmware_loader/main.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index 7c3590fd97c2..381997c84e4f 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -209,6 +209,10 @@ static void __free_fw_priv(struct kref *ref)
>  		 __func__, fw_priv->fw_name, fw_priv, fw_priv->data,
>  		 (unsigned int)fw_priv->size);
>  
> +#ifdef CONFIG_FW_LOADER_USER_HELPER
> +	list_del(&fw_priv->pending_list);
> +#endif
> +
>  	list_del(&fw_priv->list);
>  	spin_unlock(&fwc->lock);
>  

RE: [PATCH] drivers/firmware_loader: remove list entry before deallocation

[Michał Lach]

Pinging

With regards,
Michał

> -----Original Message-----
> From: Michal Lach <michal.lach@samsung.com>
> Sent: środa, 23 listopada 2022 12:15
> To: linux-kernel@vger.kernel.org
> Cc: mcgrof@kernel.org; russell.h.weight@intel.com;
> gregkh@linuxfoundation.org; rafael@kernel.org; ming.lei@canonical.com;
> tiwai@suse.de; michal.lach@samsung.com
> Subject: [PATCH] drivers/firmware_loader: remove list entry before deallocation
> 
> If CONFIG_FW_LOADER_USER_HELPER is enabled, it is possible to interrupt the
> loading process after adding pending_list to pending_fw_list.
> Subsequently, if user calls release_firmware() which deallocates the fw_priv
> structure which pending_list is a member of, the entry in the list is not deleted.
> This causes a use-after-free on further attempts to add an entry to the list or on
> list traversal.
> 
> While not problematic in most drivers since this function is mainly used in probe
> or init routines, some drivers expose firmware loading functionality via user-
> accessible functions like write() etc.
> In this case during the sysfs loading process, we can send SIGKILL to the thread
> which is then in kernel, leave the entry in the list and then free the structure.
> 
> Example kernel panics with CONFIG_DEBUG_LIST turned on:
> 
> kernel BUG at lib/list_debug.c:25!
> /* ... */
> Call trace:
>  __list_add_valid+0x7c/0x98
>  fw_load_sysfs_fallback+0xd4/0x334
>  fw_load_from_user_helper+0x148/0x1f8
>  firmware_fallback_sysfs+0xe0/0x17c
>  _request_firmware+0x1a0/0x470
>  request_firmware+0x50/0x78
> /* ... */
> 
> or
> 
> kernel BUG at lib/list_debug.c:56!
> /* ... */
> Call trace:
>  __list_del_entry_valid+0xa0/0xa4
>  fw_load_abort+0x38/0x64
>  fw_load_sysfs_fallback+0x354/0x468
>  fw_load_from_user_helper+0x17c/0x1c0
>  firmware_fallback_sysfs+0xc0/0x11c
>  _request_firmware+0xe0/0x4a4
>  request_firmware+0x20/0x2c
> /* ... */
> 
> Fixes: fe304143b0c3 ("firmware: Avoid deadlock of usermodehelper lock at
> shutdown")
> Signed-off-by: Michal Lach <michal.lach@samsung.com>
> ---
>  drivers/base/firmware_loader/main.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/base/firmware_loader/main.c
> b/drivers/base/firmware_loader/main.c
> index 7c3590fd97c2..381997c84e4f 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -209,6 +209,10 @@ static void __free_fw_priv(struct kref *ref)
>  		 __func__, fw_priv->fw_name, fw_priv, fw_priv->data,
>  		 (unsigned int)fw_priv->size);
> 
> +#ifdef CONFIG_FW_LOADER_USER_HELPER
> +	list_del(&fw_priv->pending_list);
> +#endif
> +
>  	list_del(&fw_priv->list);
>  	spin_unlock(&fwc->lock);
> 
> --
> 2.25.1

Re: [PATCH] drivers/firmware_loader: remove list entry before deallocation

[Greg KH]

On Thu, Dec 08, 2022 at 11:45:28AM +0100, Michał Lach wrote:
> Pinging

I have no context here at all.

confused,

greg k-h

Re: [PATCH] drivers/firmware_loader: remove list entry before deallocation

[Michał Lach]

On 12/8/22 14:18, Greg KH wrote:
> On Thu, Dec 08, 2022 at 11:45:28AM +0100, Michał Lach wrote:
>> Pinging
> 
> I have no context here at all.
> 
> confused,

It seems like my mail client messed up the encoding, sorry.
Below quoting the patch message:

> If CONFIG_FW_LOADER_USER_HELPER is enabled, it is possible to interrupt
> the loading process after adding pending_list to pending_fw_list.
> Subsequently, if user calls release_firmware() which deallocates the
> fw_priv structure which pending_list is a member of, the entry in the
> list is not deleted. This causes a use-after-free on further attempts
> to add an entry to the list or on list traversal.
> 
> While not problematic in most drivers since this function is mainly used
> in probe or init routines, some drivers expose firmware loading
> functionality via user-accessible functions like write() etc.
> In this case during the sysfs loading process, we can send SIGKILL to the
> thread which is then in kernel, leave the entry in the list and then free
> the structure.
> 
> Example kernel panics with CONFIG_DEBUG_LIST turned on:
> 
> kernel BUG at lib/list_debug.c:25!
> /* ... */
> Call trace:
>  __list_add_valid+0x7c/0x98
>  fw_load_sysfs_fallback+0xd4/0x334
>  fw_load_from_user_helper+0x148/0x1f8
>  firmware_fallback_sysfs+0xe0/0x17c
>  _request_firmware+0x1a0/0x470
>  request_firmware+0x50/0x78
> /* ... */
> 
> or
> 
> kernel BUG at lib/list_debug.c:56!
> /* ... */
> Call trace:
>  __list_del_entry_valid+0xa0/0xa4
>  fw_load_abort+0x38/0x64
>  fw_load_sysfs_fallback+0x354/0x468
>  fw_load_from_user_helper+0x17c/0x1c0
>  firmware_fallback_sysfs+0xc0/0x11c
>  _request_firmware+0xe0/0x4a4
>  request_firmware+0x20/0x2c
> /* ... */
> 
> Fixes: fe304143b0c3 ("firmware: Avoid deadlock of usermodehelper lock at shutdown")

On 11/29/22 20:06, Russ Weight wrote:
>> Reviewed-by: Russ Weight <russell.h.weight@intel.com>

> Signed-off-by: Michal Lach <michal.lach@samsung.com>
> ---
>  drivers/base/firmware_loader/main.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index 7c3590fd97c2..381997c84e4f 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -209,6 +209,10 @@ static void __free_fw_priv(struct kref *ref)
>  		 __func__, fw_priv->fw_name, fw_priv, fw_priv->data,
>  		 (unsigned int)fw_priv->size);
>  
> +#ifdef CONFIG_FW_LOADER_USER_HELPER
> +	list_del(&fw_priv->pending_list);
> +#endif
> +
>  	list_del(&fw_priv->list);
>  	spin_unlock(&fwc->lock);
>  
> -- 
> 2.25.1

Re: [PATCH] drivers/firmware_loader: remove list entry before deallocation

[Greg KH]

On Thu, Dec 08, 2022 at 04:23:52PM +0100, Michał Lach wrote:
> On 12/8/22 14:18, Greg KH wrote:
> > On Thu, Dec 08, 2022 at 11:45:28AM +0100, Michał Lach wrote:
> >> Pinging
> > 
> > I have no context here at all.
> > 
> > confused,
> 
> It seems like my mail client messed up the encoding, sorry.
> Below quoting the patch message:

Ok, but what does an empty ping here mean?

Are you asking why no one else has reviewed this?  Why it hasn't been
accepted?  What else needs to happen?  Something else?

be specific please :)

thanks,

greg k-h

Re: [PATCH] drivers/firmware_loader: remove list entry before deallocation

[Michał Lach]

On 12/8/22 16:42, Greg KH wrote:
> On Thu, Dec 08, 2022 at 04:23:52PM +0100, Michał Lach wrote:
>> On 12/8/22 14:18, Greg KH wrote:
>>> On Thu, Dec 08, 2022 at 11:45:28AM +0100, Michał Lach wrote:
>>>> Pinging
>>>
>>> I have no context here at all.
>>>
>>> confused,
>>
>> It seems like my mail client messed up the encoding, sorry.
>> Below quoting the patch message:
> 
> Ok, but what does an empty ping here mean?
> 
> Are you asking why no one else has reviewed this?  Why it hasn't been
> accepted?  What else needs to happen?  Something else?
> 

It was kind of meant to bump it for other reviewers to review/accept
this. Please correct me if this is against the netiquette here or 
should I just mention the reason for the ping in the first place.

> be specific please :)
> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH] drivers/firmware_loader: remove list entry before deallocation

[Greg KH]

On Thu, Dec 08, 2022 at 05:18:15PM +0100, Michał Lach wrote:
> On 12/8/22 16:42, Greg KH wrote:
> > On Thu, Dec 08, 2022 at 04:23:52PM +0100, Michał Lach wrote:
> >> On 12/8/22 14:18, Greg KH wrote:
> >>> On Thu, Dec 08, 2022 at 11:45:28AM +0100, Michał Lach wrote:
> >>>> Pinging
> >>>
> >>> I have no context here at all.
> >>>
> >>> confused,
> >>
> >> It seems like my mail client messed up the encoding, sorry.
> >> Below quoting the patch message:
> > 
> > Ok, but what does an empty ping here mean?
> > 
> > Are you asking why no one else has reviewed this?  Why it hasn't been
> > accepted?  What else needs to happen?  Something else?
> > 
> 
> It was kind of meant to bump it for other reviewers to review/accept
> this. Please correct me if this is against the netiquette here or 
> should I just mention the reason for the ping in the first place.

Please think about what you would want to see sent to you.  A "naked"
ping has no context at all, right?

How about a normal "It's been two weeks, anything else I need to do here
to get this merged?" sentance would be great.

And to help maintainer's workload, why not help out and review other
patches submitted?  That's the best way to help ensure that a
maintainer's workload is reduced, and help make your patches move to the
top of the list.

thanks,

greg k-h

Re: [PATCH] drivers/firmware_loader: remove list entry before deallocation

[Michał Lach]

On 12/8/22 19:18, Greg KH wrote:
> 
> Please think about what you would want to see sent to you.  A "naked"
> ping has no context at all, right?
> 
> How about a normal "It's been two weeks, anything else I need to do here
> to get this merged?" sentance would be great.
> 
> And to help maintainer's workload, why not help out and review other
> patches submitted?  That's the best way to help ensure that a
> maintainer's workload is reduced, and help make your patches move to the
> top of the list.
> 

Thanks, sure I will keep this in mind.

Re: [PATCH] drivers/firmware_loader: remove list entry before deallocation

[Luis Chamberlain]

Hey Michal! Thanks for your patch! I have a few doubts though!

On Wed, Nov 23, 2022 at 12:14:56PM +0100, Michal Lach wrote:
> If CONFIG_FW_LOADER_USER_HELPER is enabled, it is possible to interrupt
> the loading process after adding pending_list to pending_fw_list.

In that case wouldn't fw_load_abort() get called and do the right thing?
That in the end calls __fw_state_set() which does:

static inline void __fw_state_set(...)
{
	struct fw_state *fw_st = &fw_priv->fw_st;

	WRITE_ONCE(fw_st->status, status);
	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);
	}
}

So the question I have for you is -- what path could be cancelled
which we don't end up calling a respective abort fw_load_abort()?

> Subsequently, if user calls release_firmware() which deallocates the
> fw_priv structure which pending_list is a member of, the entry in the
> list is not deleted. This causes a use-after-free on further attempts
> to add an entry to the list or on list traversal.

Now we're dealing with the possibility of a driver interaction drivers
can be buggy too. So I'm curious what driver triggers this?

> While not problematic in most drivers since this function is mainly used
> in probe or init routines, some drivers expose firmware loading
> functionality via user-accessible functions like write() etc.
> In this case during the sysfs loading process, we can send SIGKILL to the
> thread which is then in kernel, leave the entry in the list and then free
> the structure.

To account for not having to deal with specific drivers we have the
Linux kernel selftests. And so you can test the firmware loader with all
sorts of crazy situations which any driver could use and try to see
if you can re-recreate the issue.

The kernel selftests driver for the firmware loader is in
lib/test_firmware.c and you can use thetools/testing/selftests/firmware/fw_run_tests.sh
to run all the tests. To test the firmware fallback alone you can use
just fw_fallback.sh.

If you want to just virtualize this and you can also use kdevops [0] and
enable the firmware loader selftest and use:;

make menuconfig          #  enable selftests and just the firmware test
make linux               #  build linux, pick linux-next
make selftests
make selftests-firmware

But this may be something more you can use later once you get your flow
going. Just compiling the kernel and running the selftest manually with
fw_fallback.sh should suffice.

[0] https://github.com/linux-kdevops/kdevops

> Example kernel panics with CONFIG_DEBUG_LIST turned on:
> 
> kernel BUG at lib/list_debug.c:25!
> /* ... */
> Call trace:
>  __list_add_valid+0x7c/0x98
>  fw_load_sysfs_fallback+0xd4/0x334
>  fw_load_from_user_helper+0x148/0x1f8
>  firmware_fallback_sysfs+0xe0/0x17c
>  _request_firmware+0x1a0/0x470
>  request_firmware+0x50/0x78
> /* ... */
> 
> or
> 
> kernel BUG at lib/list_debug.c:56!
> /* ... */
> Call trace:
>  __list_del_entry_valid+0xa0/0xa4
>  fw_load_abort+0x38/0x64
>  fw_load_sysfs_fallback+0x354/0x468
>  fw_load_from_user_helper+0x17c/0x1c0
>  firmware_fallback_sysfs+0xc0/0x11c
>  _request_firmware+0xe0/0x4a4
>  request_firmware+0x20/0x2c
> /* ... */

OK so this proves the bug can happen but I'd like to see the full trace
and the exact kernel version showing that this can happen on a recent
kernel. Without that I'm not seeing how this can trigger yet.

It would be easy to prove with the selftests.

  Luis

Re: [PATCH] drivers/firmware_loader: remove list entry before deallocation

[Michał Lach]

On 12/12/22 07:04, Luis Chamberlain wrote:
> Hey Michal! Thanks for your patch! I have a few doubts though!

:-)

> To account for not having to deal with specific drivers we have the
> Linux kernel selftests. And so you can test the firmware loader with all
> sorts of crazy situations which any driver could use and try to see
> if you can re-recreate the issue.
> 
> The kernel selftests driver for the firmware loader is in
> lib/test_firmware.c and you can use thetools/testing/selftests/firmware/fw_run_tests.sh
> to run all the tests. To test the firmware fallback alone you can use
> just fw_fallback.sh.
> 
> If you want to just virtualize this and you can also use kdevops [0] and
> enable the firmware loader selftest and use:;
> 
> make menuconfig          #  enable selftests and just the firmware test
> make linux               #  build linux, pick linux-next
> make selftests
> make selftests-firmware
> 
> But this may be something more you can use later once you get your flow
> going. Just compiling the kernel and running the selftest manually with
> fw_fallback.sh should suffice.

Thanks a lot, I had no idea that there is something like this.

> OK so this proves the bug can happen but I'd like to see the full trace
> and the exact kernel version showing that this can happen on a recent
> kernel. Without that I'm not seeing how this can trigger yet.

Unfortunately I cannot provide a trace. The kernel version was 5.15.41 (-stable).

Keeping that in mind, I will try to reproduce this behaviour with in-tree
code and provide proof.

With regards,
Michał

Re: [PATCH] drivers/firmware_loader: remove list entry before deallocation

[Luis Chamberlain]

On Mon, Dec 12, 2022 at 06:52:51PM +0100, Michał Lach wrote:
> On 12/12/22 07:04, Luis Chamberlain wrote:
> > Hey Michal! Thanks for your patch! I have a few doubts though!
> 
> :-)
> 
> > To account for not having to deal with specific drivers we have the
> > Linux kernel selftests. And so you can test the firmware loader with all
> > sorts of crazy situations which any driver could use and try to see
> > if you can re-recreate the issue.
> > 
> > The kernel selftests driver for the firmware loader is in
> > lib/test_firmware.c and you can use thetools/testing/selftests/firmware/fw_run_tests.sh
> > to run all the tests. To test the firmware fallback alone you can use
> > just fw_fallback.sh.
> > 
> > If you want to just virtualize this and you can also use kdevops [0] and
> > enable the firmware loader selftest and use:;
> > 
> > make menuconfig          #  enable selftests and just the firmware test
> > make linux               #  build linux, pick linux-next
> > make selftests
> > make selftests-firmware
> > 
> > But this may be something more you can use later once you get your flow
> > going. Just compiling the kernel and running the selftest manually with
> > fw_fallback.sh should suffice.
> 
> Thanks a lot, I had no idea that there is something like this.

selftests has been there for ages, kdevops support for selftests is
pretty new, like 1 week old only :)

> > OK so this proves the bug can happen but I'd like to see the full trace
> > and the exact kernel version showing that this can happen on a recent
> > kernel. Without that I'm not seeing how this can trigger yet.
> 
> Unfortunately I cannot provide a trace. The kernel version was 5.15.41 (-stable).

Ah, I suspected as much, given I couldn't see how this could happen in
linux-next.

> Keeping that in mind, I will try to reproduce this behaviour with in-tree
> code and provide proof.

Yes please use linux-next, if you can't reproduce there then likely
your kernel is missing some fixes.

  Luis