Re: [PATCH v2 8/8] Add Advantech EIO Fan driver

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Missing `.remove` callback and incomplete probe error unwinding leads to thermal zone leaks and use-after-free vulnerabilities.
- [High] Array index out-of-bounds when accessing the `fan_name` array using an unvalidated hardware index.
- [High] `eio_fan_should_bind` casts `cdev->devdata` to `struct eio_fan_dev *` without verifying the cooling device type.
- [Medium] `eio_fan_set_trip_temp` constantly fails because it incorrectly double-adds the `CTRL_THERM_HIGH` offset.
- [Medium] Passing pointers to 32-bit `int` variables for 1- or 2-byte hardware reads results in endianness corruption on big-endian architectures.
- [Medium] Violation of MFD subsystem rules by using a custom parent-child accessor function.
--

--- Patch [8]: [PATCH v2 8/8] Add Advantech EIO Fan driver ---
commit 0e6353f2bcc96ca0d70e6ec50a52311c0213d5ad
Author: Ramiro Oliveira <[email protected]>

Add Advantech EIO Fan driver

This commit adds the driver to control the Advantech EIO Fan block,
which is included in the Advantech EIO Embedded Controller.

> diff --git a/drivers/thermal/eio_fan.c b/drivers/thermal/eio_fan.c
> new file mode 100644
> index 0000000000000..e07b18c2ec44a
> --- /dev/null
> +++ b/drivers/thermal/eio_fan.c

[ ... ]

> +static int pmc_write(struct device *mfd, u8 ctrl, u8 id, void *data)
> +{
> +	if (ctrl >= ARRAY_SIZE(pmc_len))
> +		return -EINVAL;
> +
> +	struct pmc_op op = {
> +		.cmd       = CMD_FAN_WRITE,
> +		.control   = ctrl,
> +		.device_id = id,
> +		.size	   = pmc_len[ctrl],
> +		.payload   = (u8 *)data,
> +	};
> +	return eio_core_pmc_operation(mfd, &op);

[Severity: Medium]
Does using eio_core_pmc_operation() directly bypass standard kernel subsystem
APIs like regmap? According to MFD guidelines, child drivers should retrieve
parent driver data using standard APIs rather than custom parent-child
accessors.

[ ... ]

> +static int eio_fan_get_temp(struct thermal_zone_device *tzd, int *temp)
> +{
> +	struct eio_fan_dev *fan = thermal_zone_device_priv(tzd);
> +	struct device *mfd = fan->mfd;
> +	u8 ch = fan->id;
> +	int sensor = 0;
> +	u16 val = 0;
> +	int ret;
> +
> +	ret = pmc_read(mfd, CTRL_CTRL, ch, &sensor);

[Severity: Medium]
Since pmc_read() seems to read a 1-byte value based on pmc_len[CTRL_CTRL],
does passing a pointer to a 32-bit integer write only to the most-significant
byte on big-endian platforms?

[ ... ]

> +static int eio_fan_set_trip_temp(struct thermal_zone_device *tzd,
> +				 const struct thermal_trip *trip, int temp)
> +{
> +	struct eio_fan_dev *fan = thermal_zone_device_priv(tzd);
> +	const struct eio_fan_trip *fan_trip = trip->priv;
> +	u8 ctl = CTRL_THERM_HIGH + fan_trip->trip_ctl;

[Severity: Medium]
Since fan_trip->trip_ctl was already initialized to CTRL_THERM_HIGH during
probe, does adding CTRL_THERM_HIGH again cause the offset to exceed the
ARRAY_SIZE(pmc_len) limit of 33, making pmc_write() always return -EINVAL?

[ ... ]

> +static bool eio_fan_should_bind(struct thermal_zone_device *tzd,
> +				const struct thermal_trip *trip,
> +				struct thermal_cooling_device *cdev,
> +				struct cooling_spec *spec)
> +{
> +	struct eio_fan_dev *tz_fan  = thermal_zone_device_priv(tzd);
> +	struct eio_fan_dev *cd_fan  = cdev->devdata;
> +
> +	if (!tz_fan || !cd_fan)
> +		return false;
> +
> +	if (tz_fan->mfd != cd_fan->mfd || tz_fan->id != cd_fan->id)

[Severity: High]
Is it safe to cast cdev->devdata to struct eio_fan_dev* here without first
verifying that the cooling device actually belongs to this driver (for
example, by checking cdev->ops)? Could this lead to out-of-bounds memory
access if the thermal framework invokes this for a cooling device belonging to
another driver?

[ ... ]

> +static int eio_fan_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	unsigned int fan_id;
> +	int ret;
> +
> +	if (!dev_get_drvdata(dev->parent)) {
> +		dev_err(dev, "eio_core not present\n");
> +		return -ENODEV;
> +	}
> +
> +	for (fan_id = 0; fan_id < FAN_MAX; fan_id++) {
> +		u8 state = 0, name = 0;
> +		int trip_hi = 0, trip_lo = 0, trip_stop = 0;
> +		int pwm_hi = 0, pwm_lo = 0;
> +		struct eio_fan_dev *fan;
> +		struct thermal_zone_device *tzd;
> +		struct thermal_cooling_device *cdev;
> +
> +		if (pmc_read(dev->parent, CTRL_STATE, fan_id, &state) ||
> +		    pmc_read(dev->parent, CTRL_TYPE, fan_id, &name) ||
> +		    pmc_read(dev->parent, CTRL_THERM_HIGH, fan_id, &trip_hi) ||

[Severity: Medium]
Similar to eio_fan_get_temp(), does passing pointers to 32-bit integers for
these hardware reads cause endianness corruption on big-endian architectures?

[ ... ]

> +		if (!(state & 0x1)) {
> +			dev_info(dev, "fan%u: firmware reports disabled\n", fan_id);
> +			continue;
> +		}
> +
> +		if (!fan_name[name][0]) {

[Severity: High]
The 'name' index is read from hardware as a u8. Does this code need to check
if 'name' exceeds the 32-element bounds of the fan_name[] array to prevent an
out-of-bounds read?

[ ... ]

> +		cdev = thermal_cooling_device_register(fan_name[name], fan, &cooling_ops);
> +		if (IS_ERR(cdev)) {
> +			thermal_zone_device_unregister(tzd);
> +			dev_err(dev, "fan%u: cdev register failed: %ld\n",
> +				fan_id, PTR_ERR(cdev));
> +			return PTR_ERR(cdev);
> +		}

[Severity: High]
If this loop fails on a subsequent fan iteration, does returning directly
here leave previously registered thermal zones and cooling devices active?
Should this failure path unwind the already registered instances?

[ ... ]

> +static struct platform_driver eio_fan_driver = {
> +	.probe  = eio_fan_probe,
> +	.driver = {
> +		.name = "eio_fan",
> +	},
> +};

[Severity: High]
Does the lack of a .remove callback mean that unbinding the driver via sysfs
abandons the registered thermal zones and cooling devices? Because
devm_kzalloc() frees the memory on unbind, could this leave dangling pointers
in the thermal subsystem?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8