Re: [PATCH v3 2/2] scsi: mpt3sas: add hwmon support

[sashiko-bot@kernel.org]

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

New issues:
- [High] Local variables `mpi_reply` and `page` are allocated on the stack without initialization before being passed to `mpt3sas_config_get_iounit_pg7()`.

Pre-existing issues:
- [High] Data race on `ioc->config_cmds.status` between process context timeout path and interrupt context reply handler, which can permanently break firmware config requests.
--

commit 0a8c15a13aecbcd080b4e88615e9f0ee4d3be366
Author: Louis Sautier <sautier.louis@gmail.com>

scsi: mpt3sas: add hwmon support

This patch adds hwmon support to expose the IOC and board temperature sensors
of LSI/Broadcom SAS HBAs. It reads sensor data from MPI IO Unit Page 7 and
registers an hwmon device.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_hwmon.c b/drivers/scsi/mpt3sas/mpt3sas_hwmon.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/scsi/mpt3sas/mpt3sas_hwmon.c

[ ... ]

> +static int
> +_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> +	    u32 attr, int channel, long *val)
> +{
> +	struct mpt3sas_hwmon *h = dev_get_drvdata(dev);
> +	Mpi2ConfigReply_t mpi_reply;
> +	Mpi2IOUnitPage7_t page;

[Severity: High]
Could these uninitialized stack variables lead to memory corruption or
information leaks?

If the firmware completes the config request without a reply frame payload,
the underlying _config_request() returns 0 but skips populating mpi_reply.
The subsequent config read request relies on mpi_reply.Header.PageLength
and mpi_reply.ExtPageLength to compute the DMA memory allocation size.
Using uninitialized stack garbage for the size might lead to unpredictable
dma_alloc_coherent() behavior.

Additionally, if page is not fully initialized by the hardware response,
stack garbage might be returned to userspace via _hwmon_to_mdegc().
Should these structs be zero-initialized?

> +	int r;
> +
> +	if (type != hwmon_temp || attr != hwmon_temp_input)
> +		return -EOPNOTSUPP;
> +
> +	r = mpt3sas_config_get_iounit_pg7(h->ioc, &mpi_reply, &page);

[Severity: High]
This is a pre-existing issue, but does calling mpt3sas_config_get_iounit_pg7()
expose a data race with the config_cmds state machine?

In the underlying _config_request() code, there appears to be a data race on
ioc->config_cmds.status between the process context timeout path and the
interrupt context reply handler (mpt3sas_config_done).

If wait_for_completion_timeout() expires in _config_request(), it resets the
state machine:

    ioc->config_cmds.status = MPT3_CMD_NOT_USED;

If a late firmware reply concurrently triggers mpt3sas_config_done() in an
interrupt context, it uses non-atomic read-modify-write operations:

    ioc->config_cmds.status |= MPT3_CMD_COMPLETE;
    ioc->config_cmds.status &= ~MPT3_CMD_PENDING;

If _config_request() writes MPT3_CMD_NOT_USED during the interrupt's
read-modify-write cycle, the interrupt's write can clobber it, leaving the
status as MPT3_CMD_COMPLETE.

Future calls to _config_request() check if the status is MPT3_CMD_NOT_USED
under a mutex and would return -EAGAIN, permanently locking out all future
config requests for this adapter. Can this be protected with a spinlock or
by using atomic bitops?

> +	if (r)
> +		return r;

[ ... ]

> +int
> +mpt3sas_hwmon_register(struct MPT3SAS_ADAPTER *ioc)
> +{
> +	struct device *parent = &ioc->pdev->dev;
> +	struct mpt3sas_hwmon *h;
> +	struct device *hwdev;
> +	Mpi2ConfigReply_t mpi_reply;
> +	Mpi2IOUnitPage7_t page;

[Severity: High]
Similarly here, can the uninitialized mpi_reply and page variables cause
the same issues described above during the config page request?

> +	int r;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260524210545.1333637-1-sautier.louis@gmail.com?part=2