Re: [PATCH AUTOSEL 6.18-5.10] scsi: pm8001: Reject firmware update in fatal error state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] TOCTOU race in fw_status check allows concurrent firmware updates, leading to a double free and Use-After-Free (UAF) of pm8001_ha->fw_image.
--

commit 0afa89048afde066ccd69e2cb178118dd117d047
Author: Kumar Meiyappan <kumar.meiyappan@microchip.com>

scsi: pm8001: Reject firmware update in fatal error state

pm8001_store_update_fw() allows a firmware update request even when the
controller has already entered a fatal error state. This commit rejects the
request early by checking controller_fatal_error to avoid a call trace.

Link: https://patch.msgid.link/20260416153757.414896-1-sagar.biradar@microchip.com

> diff --git a/drivers/scsi/pm8001/pm8001_ctl.c b/drivers/scsi/pm8001/pm8001_ctl.c
> index cbfda8c04e956..bb38b2d63acb8 100644
> --- a/drivers/scsi/pm8001/pm8001_ctl.c
> +++ b/drivers/scsi/pm8001/pm8001_ctl.c
> @@ -826,6 +826,14 @@ static ssize_t pm8001_store_update_fw(struct device *cdev,
>  		goto out;
>  	}
>  
> +	if (pm8001_ha->controller_fatal_error) {
> +		pm8001_dbg(pm8001_ha, FAIL,
> +			   "controller in fatal error state, firmware update rejected\n");
> +		pm8001_ha->fw_status = FAIL_PARAMETERS;
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +

[Severity: High]
This isn't a bug introduced by this patch, but could this sysfs write handler
be subject to a TOCTOU race condition if multiple threads with CAP_SYS_ADMIN
privileges write to the update_fw sysfs attribute concurrently?

Because kernfs does not serialize concurrent writes to the same sysfs file,
the check earlier in pm8001_store_update_fw():

	if (pm8001_ha->fw_status == FLASH_IN_PROGRESS)
		return -EINPROGRESS;
	pm8001_ha->fw_status = FLASH_IN_PROGRESS;

lacks lock protection. If two threads pass this check simultaneously, they will
both proceed and later call request_firmware() on the same pointer:

	ret = request_firmware(&pm8001_ha->fw_image,
			       filename_ptr,
			       pm8001_ha->dev);

This would leak the first firmware allocation. Furthermore, both threads will
eventually call:

	release_firmware(pm8001_ha->fw_image);

Does this sequence result in a double free of pm8001_ha->fw_image?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831133314.4125787-555-sashal@kernel.org?part=1