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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unprivileged user-space processes can flood the HBA firmware with synchronous configuration requests, contending a shared mutex and causing a Denial of Service (DoS).
--

commit 12785f6156b64bc1ece9061e087614ba8f75d567
Author: Louis Sautier <sautier.louis@gmail.com>

scsi: mpt3sas: add hwmon support

This commit adds hardware monitoring support for LSI / Broadcom SAS HBAs,
exposing IOC and board temperature sensors. The readings are obtained by
querying MPI IO Unit Page 7 and exposed to userspace through standard
hwmon sysfs files.

> 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 umode_t
> +_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
> +		  u32 attr, int channel)
> +{
[ ... ]
> +	if (channel == 0 && h->ioc_present)
> +		return 0444;
> +	if (channel == 1 && h->board_present)
> +		return 0444;
> +	return 0;
> +}
[ ... ]
> +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;
> +	int r;
> +
> +	if (type != hwmon_temp || attr != hwmon_temp_input)
> +		return -EOPNOTSUPP;
> +
> +	r = mpt3sas_config_get_iounit_pg7(h->ioc, &mpi_reply, &page);

Could exposing this read to unprivileged users lead to excessive firmware
requests and mutex contention?

Since the sysfs files are world-readable, any local user can read them in a
tight loop. Every read directly calls mpt3sas_config_get_iounit_pg7(), which
issues synchronous requests to the firmware over the PCIe bus and acquires
the shared config_cmds.mutex:

_hwmon_read() ->
    mpt3sas_config_get_iounit_pg7() ->
        _config_request() ->
            mutex_lock(&ioc->config_cmds.mutex)

Does this allow an unprivileged user to starve other storage subsystem
components that rely on this mutex for administrative tasks or device
discovery?

Could we implement a caching mechanism (for example, caching the readings
for a short period using jiffies) to rate-limit these firmware requests?

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