Re: [PATCH 28/37] scsi: mpt3sas: Replace panic() calls with error returns in _config_request

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: mpt3sas: corrupted mpi_reply used in NVRAM write after -EIO return
--

commit 2d94d7e58bda28658da04f2a639f8f08b1c34536
Author: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>

scsi: mpt3sas: Replace panic() calls with error returns in _config_request

This commit replaces four panic() calls in _config_request() triggered by
firmware reply or configuration page-type mismatches with ioc_err() logs
and -EIO error returns.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_config.c b/drivers/scsi/mpt3sas/mpt3sas_config.c
> index 2b5e42255d3b5..271d7dff7c079 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_config.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_config.c
> @@ -416,10 +416,13 @@ _config_request(struct MPT3SAS_ADAPTER *ioc, Mpi2ConfigRequest_t
>  				    smid, "config_request", NULL);
>  			_debug_dump_mf(mpi_request, ioc->request_sz/4);
>  			_debug_dump_reply(mpi_reply, ioc->reply_sz/4);
> -			panic("%s: %s: Firmware BUG: mpi_reply mismatch: Requested PageType(0x%02x) Reply PageType(0x%02x)\n",
> -			      ioc->name, __func__,
> -			      mpi_request->Header.PageType & 0xF,
> -			      mpi_reply->Header.PageType & 0xF);
> +			ioc_err(ioc,
> +				"%s: Firmware BUG: mpi_reply mismatch: Requested PageType(0x%02x) Reply PageType(0x%02x)\n",
> +				__func__,
> +				mpi_request->Header.PageType & 0xF,
> +				mpi_reply->Header.PageType & 0xF);
> +			r = -EIO;
> +			goto free_mem;
>  		}
>  
>  		if (((mpi_request->Header.PageType & 0xF) ==
> @@ -430,10 +433,13 @@ _config_request(struct MPT3SAS_ADAPTER *ioc, Mpi2ConfigRequest_t
>  				    smid, "config_request", NULL);
>  			_debug_dump_mf(mpi_request, ioc->request_sz/4);
>  			_debug_dump_reply(mpi_reply, ioc->reply_sz/4);
> -			panic("%s: %s: Firmware BUG: mpi_reply mismatch: Requested ExtPageType(0x%02x) Reply ExtPageType(0x%02x)\n",
> -			      ioc->name, __func__,
> -			      mpi_request->ExtPageType,
> -			      mpi_reply->ExtPageType);
> +			ioc_err(ioc,
> +				"%s: Firmware BUG: mpi_reply mismatch: Requested ExtPageType(0x%02x) Reply ExtPageType(0x%02x)\n",
> +				__func__,
> +				mpi_request->ExtPageType,
> +				mpi_reply->ExtPageType);
> +			r = -EIO;
> +			goto free_mem;
>  		}

[Severity: High]
Can returning -EIO here after overwriting mpi_reply cause subsequent
NVRAM writes to target the wrong configuration page?

In _config_request(), the caller's mpi_reply buffer is overwritten before
the PageType and ExtPageType sanity checks run:

	if (ioc->config_cmds.status & MPT3_CMD_REPLY_VALID) {
		memcpy(mpi_reply, ioc->config_cmds.reply,
		    sizeof(Mpi2ConfigReply_t));

Previously, a mismatch here called panic(). With -EIO returned instead,
callers such as mpt3sas_config_set_sas_iounit_pg1() and
_config_set_driver_trigger_pg0() through _config_set_driver_trigger_pg4()
do not check the return value of the MPI2_CONFIG_ACTION_PAGE_WRITE_CURRENT
call before issuing MPI2_CONFIG_ACTION_PAGE_WRITE_NVRAM with the same
mpi_reply:

	mpi_request.Action = MPI2_CONFIG_ACTION_PAGE_WRITE_CURRENT;
	_config_request(ioc, &mpi_request, mpi_reply,
	    MPT3_CONFIG_PAGE_DEFAULT_TIMEOUT, config_page, sz);
	mpi_request.Action = MPI2_CONFIG_ACTION_PAGE_WRITE_NVRAM;
	r = _config_request(ioc, &mpi_request, mpi_reply,
	    MPT3_CONFIG_PAGE_DEFAULT_TIMEOUT, config_page, sz);

When the second _config_request() call begins with a non-NULL config_page,
it copies the header fields from the mismatched mpi_reply into mpi_request:

	if (config_page) {
		mpi_request->Header.PageVersion = mpi_reply->Header.PageVersion;
		mpi_request->Header.PageNumber = mpi_reply->Header.PageNumber;
		mpi_request->Header.PageType = mpi_reply->Header.PageType;
		mpi_request->Header.PageLength = mpi_reply->Header.PageLength;
		mpi_request->ExtPageLength = mpi_reply->ExtPageLength;
		mpi_request->ExtPageType = mpi_reply->ExtPageType;

Wouldn't this send the MPI2_CONFIG_ACTION_PAGE_WRITE_NVRAM request to
firmware with the corrupted page header from the mismatched reply?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/1793925772-1303-1-git-send-email-shivasharan.srikanteshwara@broadcom.com?part=28