Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <97ae8658-4eca-61af-5d5b-21b958ce1c24@samsung.com>
Date:   Thu, 8 Dec 2022 16:23:52 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
        Thunderbird/102.4.2
From:   =?UTF-8?Q?Micha=c5=82_Lach?= <michal.lach@samsung.com>
Subject: Re: [PATCH] drivers/firmware_loader: remove list entry before
 deallocation
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     linux-kernel@vger.kernel.org, mcgrof@kernel.org,
        russell.h.weight@intel.com, rafael@kernel.org,
        ming.lei@canonical.com, tiwai@suse.de
Content-Language: en-US
In-Reply-To: <Y5HkIl41zN9fwKV8@kroah.com>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <CGME20221123111806eucas1p23fdcdbe6e5f4a9e714db428fcd6552b9@eucas1p2.samsung.com>
        <20221123111455.94972-1-michal.lach@samsung.com>
        <000901d90af2$309b7c80$91d27580$@samsung.com> <Y5HkIl41zN9fwKV8@kroah.com>
Precedence: bulk

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