linux-kernel - Re: [PATCH v2] scsi: ufs: fix use-after free in init error and remove paths

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] [day] [month] [year] [list]

Message-ID: <cec304ef-0a5d-4f80-aaa4-05432c7a0b88@acm.org>
Date: Tue, 14 Jan 2025 12:41:34 -0800
From: Bart Van Assche <bvanassche@....org>
To: André Draszik <andre.draszik@...aro.org>,
 Alim Akhtar <alim.akhtar@...sung.com>, Avri Altman <avri.altman@....com>,
 "James E.J. Bottomley" <James.Bottomley@...senPartnership.com>,
 "Martin K. Petersen" <martin.petersen@...cle.com>,
 Peter Griffin <peter.griffin@...aro.org>,
 Krzysztof Kozlowski <krzk@...nel.org>,
 Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
 Eric Biggers <ebiggers@...nel.org>
Cc: Tudor Ambarus <tudor.ambarus@...aro.org>,
 Will McVicker <willmcvicker@...gle.com>, kernel-team@...roid.com,
 linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-samsung-soc@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-arm-msm@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v2] scsi: ufs: fix use-after free in init error and remove
 paths

On 1/14/25 11:56 AM, André Draszik wrote:
> Hi Bart,
> 
> On Tue, 2025-01-14 at 09:55 -0800, Bart Van Assche wrote:
>> On 1/14/25 8:16 AM, André Draszik wrote:
>>> +/**
>>> + * ufshcd_scsi_host_put_callback - deallocate underlying Scsi_Host and
>>> + *				   thereby the Host Bus Adapter (HBA)
>>> + * @host: pointer to SCSI host
>>> + */
>>> +static void ufshcd_scsi_host_put_callback(void *host)
>>> +{
>>> +	scsi_host_put(host);
>>> +}
>>
>> Please rename ufshcd_scsi_host_put_callback() such that the function
>> name makes clear when this function is called instead of what the
>> function does.
> 
> Would you have a suggestion for such a name? Something like
> ufshcd_driver_release_action()?
> 
> Unless I'm misunderstanding you, I believe most drivers use
> a function name that says what the function does, e.g.
> dell_wmi_ddv_debugfs_remove (just as a completely random
> example out of many).
> 
> If going by when it is called and if applying this principle
> throughout ufshcd, then there can only ever be one such
> function in ufshcd, as all devm_add_action() callback actions
> happen at driver release, which surely isn't what you mean.
> 
> You probably meant something different?

I meant what I wrote in my previous email: to chose another name
for ufshcd_scsi_host_put_callback() only. Having a function name that
duplicates the function body leaves readers of the code guessing
from where the function is called. BTW, naming callbacks after their
call site is a normal practice as far as I know. From ufs-qcom.c:

static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
	.name                   = "qcom",
	.init                   = ufs_qcom_init,
	.exit                   = ufs_qcom_exit,
	.get_ufs_hci_version	= ufs_qcom_get_ufs_hci_version,
	.clk_scale_notify	= ufs_qcom_clk_scale_notify,
	.setup_clocks           = ufs_qcom_setup_clocks,
	.hce_enable_notify      = ufs_qcom_hce_enable_notify,
	.link_startup_notify    = ufs_qcom_link_startup_notify,
	.pwr_change_notify	= ufs_qcom_pwr_change_notify,
	.apply_dev_quirks	= ufs_qcom_apply_dev_quirks,
	.fixup_dev_quirks       = ufs_qcom_fixup_dev_quirks,
	.suspend		= ufs_qcom_suspend,
	.resume			= ufs_qcom_resume,
	.dbg_register_dump	= ufs_qcom_dump_dbg_regs,
	.device_reset		= ufs_qcom_device_reset,
	.config_scaling_param = ufs_qcom_config_scaling_param,
	.reinit_notify		= ufs_qcom_reinit_notify,
	.mcq_config_resource	= ufs_qcom_mcq_config_resource,
	.get_hba_mac		= ufs_qcom_get_hba_mac,
	.op_runtime_config	= ufs_qcom_op_runtime_config,
	.get_outstanding_cqs	= ufs_qcom_get_outstanding_cqs,
	.config_esi		= ufs_qcom_config_esi,
};

Bart.