Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider: - [High] Off-by-one boundary check in `check_pin()` allows out-of-bounds array read of `group_map`. - [High] Integer shift overflow when generating a 64-bit mask using the `BIT()` macro on 32-bit architectures. - [Medium] The supported pins bitmask (`avail`) is calculated but never enforced, allowing interaction with unsupported pins. - [Medium] Failure to check the return value of `pmc_write` allows silent failures when configuring pin direction. - [Medium] Meaningful error codes returned by `gpio_init()` are squashed to `-EIO` in `gpio_probe()`. - [Medium] Setting GPIO direction to output before configuring the output level causes a physical hardware glitch. - [Medium] Endianness mismatch when reading hardware registers byte-by-byte into a multi-byte integer address. - [Low] Off-by-one boundary check in `pmc_read()` allows out-of-bounds array read. - [Low] Returning `-ENOMEM` instead of `-EINVAL` for invalid control arguments in `pmc_read` and `pmc_write`. --
--- Patch [2]: [PATCH v2 2/8] Add Advantech EIO GPIO driver --- commit 22717cbc8cf200661da102ffdd6bee43f768b719 Author: Ramiro Oliveira <[email protected]> Add Advantech EIO GPIO driver This driver controls the GPIO component of the Advantech EIO chip. > diff --git a/drivers/gpio/gpio-eio.c b/drivers/gpio/gpio-eio.c > new file mode 100644 > index 0000000000000..34e1aefd0716c > --- /dev/null > +++ b/drivers/gpio/gpio-eio.c [ ... ] > +static int pmc_write(struct device *mfd_dev, u8 ctrl, u8 dev_id, void *data) > +{ > + struct pmc_op op = { > + .cmd = EIO_GPIO_WRITE, > + .control = ctrl, > + .device_id = dev_id, > + .payload = (u8 *)data, > + }; > + > + if (ctrl >= ARRAY_SIZE(ctrl_para)) > + return -ENOMEM; [Severity: Low] Should this bounds check return -EINVAL instead of -ENOMEM? Returning -ENOMEM signifies a memory allocation failure to the caller, but no allocation is occurring here. [ ... ] > +static int pmc_read(struct device *mfd_dev, u8 ctrl, u8 dev_id, void *data) > +{ > + struct pmc_op op = { > + .cmd = EIO_GPIO_READ, > + .control = ctrl, > + .device_id = dev_id, > + .payload = (u8 *)data, > + }; > + > + if (ctrl > ARRAY_SIZE(ctrl_para)) > + return -ENOMEM; [Severity: Low] Is there an off-by-one error in this boundary check? Using greater-than allows an input exactly equal to the array size to bypass the check, which could lead to an out-of-bounds read on the following line. [Severity: Low] Additionally, should this return -EINVAL instead of -ENOMEM when the control argument is invalid? [ ... ] > +static int dir_output(struct gpio_chip *chip, unsigned int offset, int value) > +{ > + u8 dir = 1; > + u8 val = value; > + > + pmc_write(chip->parent, EIO_GPIO_PIN_DIR, offset, &dir); > + > + return pmc_write(chip->parent, EIO_GPIO_PIN_LEVEL, offset, &val); > +} [Severity: Medium] Does setting the GPIO direction to output before configuring the output level cause a physical hardware glitch? Every time a pin is reconfigured as an output, it might briefly drive whatever stale default value resides in its output latch until the second write command completes. [Severity: Medium] Does this also silently discard the return value of the initial direction configuration? If the first pmc_write() call fails due to an I/O communication error, the pin direction may remain misconfigured as an input while the driver falsely reports success and proceeds to drive the level. [ ... ] > +static int check_pin(struct device *dev, int pin) > +{ > + int ret; > + int group, bit; > + u16 data; > + > + /* Get pin mapping */ > + ret = pmc_read(dev, EIO_GPIO_MAPPING, pin, &data); [Severity: Medium] Does reading hardware registers byte-by-byte into a multi-byte integer introduce an endianness mismatch? When pmc_read() iterates and places bytes sequentially into the target address, it assumes a Little-Endian memory layout. On Big-Endian platforms, the multi-byte integer will be assembled with the wrong byte order. > + if (ret) > + return ret; > + > + if ((data & 0xFF) > ARRAY_SIZE(group_map)) > + return -EINVAL; [Severity: High] Is there an off-by-one boundary check here that could allow an out-of-bounds array read? If the hardware embedded controller returns a mapping value where (data & 0xFF) exactly equals the array size, it would bypass this check and cause an out-of-bounds access on group_map in the next line. > + > + group = group_map[data & 0xFF].group; > + bit = data >> 8; > + > + /* Check mapped pin */ > + ret = pmc_read(dev, EIO_GPIO_GROUP_AVAIL, group, &data); > + if (ret) > + return ret; > + > + return data & BIT(bit) ? 0 : -EOPNOTSUPP; [Severity: High] Can using the unvalidated hardware value for the bit shift amount cause undefined behavior on 32-bit architectures? [ ... ] > +static int gpio_init(struct device *mfd, struct eio_gpio_dev *eio_gpio) > +{ > + int ret, i; > + > + ret = check_support(mfd); > + if (ret) > + return dev_err_probe(eio_gpio->dev, ret, "GPIO not > supported\n"); > + > + eio_gpio->avail = 0; > + > + for (i = 0 ; i < EIO_GPIO_MAX_PINS ; i++) { > + ret = check_pin(mfd, i); > + if (ret) > + continue; > + > + eio_gpio->avail |= BIT(i); [Severity: High] Does this cause an integer shift overflow when generating the 64-bit mask? Since EIO_GPIO_MAX_PINS is 48, using the 32-bit BIT() macro will shift by 32 or more on 32-bit systems, causing undefined behavior. Should this use BIT_ULL() instead? [ ... ] > +static int gpio_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct eio_gpio_dev *eio_gpio; > + struct eio_dev *eio_dev = dev_get_drvdata(dev->parent); > + > + if (!eio_dev) > + return dev_err_probe(dev, -ENODEV, "Error contact eio_core\n"); > + > + eio_gpio = devm_kzalloc(dev, sizeof(*eio_gpio), GFP_KERNEL); > + if (!eio_gpio) > + return -ENOMEM; > + > + eio_gpio->dev = dev; > + > + if (gpio_init(dev->parent, eio_gpio)) > + return -EIO; [Severity: Medium] Is it intentional to discard the meaningful error codes returned by gpio_init() and replace them with -EIO? This might disrupt the standard kernel error propagation, specifically breaking deferred probing if gpio_init() returns -EPROBE_DEFER. > + > + eio_gpio->chip.parent = dev->parent; > + eio_gpio->chip.ngpio = eio_gpio->max; [Severity: Medium] The supported pins bitmask eio_gpio->avail is computed during gpio_init() but never seems to be enforced. Does the gpio_chip need to be populated with an .init_valid_mask callback so that gpiolib enforces the valid pins mask? Otherwise, gpiolib will implicitly assume all pins in the range are valid, and unsupported pins could be accessed. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
