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
