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: <11e3bb03-ae3f-6522-2a72-23d9f7cc3956@intel.com>
Date:   Tue, 29 Nov 2022 11:06:57 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.11.0
Subject: Re: [PATCH] drivers/firmware_loader: remove list entry before
 deallocation
Content-Language: en-US
To:     Michal Lach <michal.lach@samsung.com>,
        <linux-kernel@vger.kernel.org>
CC:     <mcgrof@kernel.org>, <gregkh@linuxfoundation.org>,
        <rafael@kernel.org>, <ming.lei@canonical.com>, <tiwai@suse.de>
References: <CGME20221123111806eucas1p23fdcdbe6e5f4a9e714db428fcd6552b9@eucas1p2.samsung.com>
 <20221123111455.94972-1-michal.lach@samsung.com>
From:   Russ Weight <russell.h.weight@intel.com>
In-Reply-To: <20221123111455.94972-1-michal.lach@samsung.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?OUpBZFJxalp6L1FnUWhaamhyd0hKNzZlVVh2OWhtQm9MOG1wQmtIN2RzaWcy?=
 =?utf-8?B?MmNpUnA2VUdyT2Q2N2NLNlc1TEJPSGl6TXFxUlI2b1hrMm5adzNpL2d0WW5V?=
 =?utf-8?B?U1JCOHZMcW5KTVV3U1lPSTEzdWRteHU5bTg1d3BHWk1VSDEwUVZsV2Q3OGtV?=
 =?utf-8?B?YXptcnNMRUlZOHpSVzNaaFF0YkVLeXdFdnB0M1ppbm1YcnNYRkdIVlNvZk5R?=
 =?utf-8?B?dzRpb0lDMVk4UmJlTXp6dSt2cVN3MnNvdEFmbmplYnE2enc3a1FwcHR0bUxj?=
 =?utf-8?B?ZndjQUdWb3RJSlUvb2UzVUs5Vm9yMG00aHZ5R0xMVVJJa3o4RXpqeS9BMHQ1?=
 =?utf-8?B?NllvdXFvNUJiOHdUd2VTQUl6RVJBQmhURmxqaXNMYm9UQkVSTEdWaEk0WEdj?=
 =?utf-8?B?UXRPNjUzV3RWdEhBS0NyY0ppOWJReU5VVm5HMlhSdHJhZFRXU1VEVjg5WUtO?=
 =?utf-8?B?dXltSldzTnNOc1U3UDlFS1VqSjdrTm5qYndtNXA3YUF0dlBKZnllU1ZGMGkv?=
 =?utf-8?B?UEdscTE5dVY1a2gvcXpOaisxZ24xZFZnVGhPTm9LMGU1dG5xU1V5Q0U2d2dp?=
 =?utf-8?B?UlRVbEJnbFkrRWhqb3h2SVAya2V5cklIODZUVXpjVndnUVNNWEk0N2lmbm9i?=
 =?utf-8?B?cURnZ0dMK25hdGxMWnViUHlwVlB1TUxEUXFKSkVpNWRyTEdHMisyMER3RjI1?=
 =?utf-8?B?WVJoYVBrVjR3eTRoUUNySDB6cXVJWTlRSjlhWGdwTHBvRHlQSjdnY1d4SXFr?=
 =?utf-8?B?NS9INTIrNy93YWxUNWlnbmV2R3lPNllLRStmb3JJY2lmU1I5VEtRcnJEb2R2?=
 =?utf-8?B?amx2VTc5Um9oUFB6QUNnNjRGZlBLcEsxeDdvdWVweHhFQ3REeHY0MThTWlhw?=
 =?utf-8?B?cGV2NHVZMHl4MUIxa1IwQ2o0cmxuaHNMR2E2cURxMkJXK3krWC9RRTA2cm9i?=
 =?utf-8?B?V0FTVVVHNXFmbTJqWUJEVXgwOWN0SHNDMDlRbjFBUGlWWkZyemk0WUNJdy8w?=
 =?utf-8?B?azJlcWFjUExBL0owYm0zNTczMlVDL3VObkZYTjNXaWRKUHExRHVpd2NvRnRt?=
 =?utf-8?B?S1JPa3B6cXlJeTk2ZCt1b3JYd0V3QjBOR3NOL3M1WmxFNDBadnkzK2YxTVo4?=
 =?utf-8?B?bkJOV3FLQlVlck5QNDlWQ2NieFZ1S245bXNhVlRSTlhaZExCUnlEb1JjdTJu?=
 =?utf-8?B?MlRLTm9Rdm1LK2RPNDluQzBQcXF5NzNLdXd5aklQdWVnYXVOK1pFaXFPQjJC?=
 =?utf-8?B?bnQ5anBDZk9vTHZmVlEvdkJhZU1ZRTB1dzQwdGZiVXFzclNWdmxxRzVtOERY?=
 =?utf-8?B?bVJXRmdOenpWbTNaY2lDSUQwcVJ5Zmd6d0YwQk95MFpqdTd4b0Q2bVFwTG10?=
 =?utf-8?B?TjVwT2NSRmZzcTlvV3hXaFV2TkZHVG9ZQzQrR3lUVmdzWXBaMzNlVWZ2bHhU?=
 =?utf-8?B?bVNYY3A2dG02RnM0R2FVa0srbEkxSm9MTHh0Rkl3WXlhM3ZPVFFCbFp2V2ly?=
 =?utf-8?B?WVBWK0hkcU1JQzlGbG9JYWNTZ3BjZ0RzQmVhU0NuMnh6WkNLZXhINDFJWitt?=
 =?utf-8?B?cE9vYXB2TnpIWkVnRkZBcWswODVPdFQyS295RFUycnlYa3JrTkFNSDU0cmJB?=
 =?utf-8?B?eTl4eUY3czl6aHZoMzRJZUN1a0lrWTNQSFZTYUswVmExby9oaytDYUQ2SFZp?=
 =?utf-8?B?Q3B2OVpYV2xwdldqYWx2bUZaU1N1UmdETFVLM0hiWHdBR3pXUXQ1b0lxTFpZ?=
 =?utf-8?B?R0pDeUduT3FaeDZhMXZKcjNpdndRUXp5RkkvUTh4ZWtnRVhSMmQ4MlB6RDUz?=
 =?utf-8?B?UG5CaCtmRnZxMXVram91WXlCTDR3alpaNko3WEcxeTl0MzRBRnZXem5SWFc5?=
 =?utf-8?B?RmFZeXlRYTZiZnlEdkJ3UmhkeTIwQ3ZhTFREa2Zvb1I4RmJzZ2JNM3NmVGVQ?=
 =?utf-8?B?aFhSZG9HLzdPcmxBVkVrcGc4dmxnWEJZZVpRaEIzUjA4bFgwNXdEYk1ZNk1T?=
 =?utf-8?B?QnJscm05ODhyVnl3YlVIZ2pncjZzNkNHMXJjWmoyY0xhQkg1WE9ibEtWcVl4?=
 =?utf-8?B?SkVYM2VlKys4T0oxUm5QM3o5QlE0ODYxQTU3MTBaRzdna1BRL0JQUFUxOEVH?=
 =?utf-8?B?OGJQVFlFWkZ2TXYrdGJQend1cW1OYWp5R2lhSk5jN2dDZFQ2R3FFS2RwbU1z?=
 =?utf-8?B?dUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 61ff5a51-0b64-4363-5ccc-08dad23ceaf2
X-MS-Exchange-CrossTenant-AuthSource: DM5PR11MB1899.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Nov 2022 19:07:11.9062
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: oQmyJC/fZuuhrbW1APx9ME8u+I42r/4SOSNcxuMvT8rrC6sG4ijujO//aU5UsYCWdBNtzbG2x3ebdG9tDZz7vgFKuaCkTNG7ESIKv5yjIek=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB7397
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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);
>