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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <000901d90af2$309b7c80$91d27580$@samsung.com>
Date:   Thu, 8 Dec 2022 11:45:28 +0100
From:   Michał Lach <michal.lach@...sung.com>
To:     <linux-kernel@...r.kernel.org>
Cc:     <mcgrof@...nel.org>, <russell.h.weight@...el.com>,
        <gregkh@...uxfoundation.org>, <rafael@...nel.org>,
        <ming.lei@...onical.com>, <tiwai@...e.de>
Subject: RE: [PATCH] drivers/firmware_loader: remove list entry before
 deallocation

Pinging

With regards,
Michał

> -----Original Message-----
> From: Michal Lach <michal.lach@...sung.com>
> Sent: środa, 23 listopada 2022 12:15
> To: linux-kernel@...r.kernel.org
> Cc: mcgrof@...nel.org; russell.h.weight@...el.com;
> gregkh@...uxfoundation.org; rafael@...nel.org; ming.lei@...onical.com;
> tiwai@...e.de; michal.lach@...sung.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@...sung.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