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

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