Re: [PATCH 2/2] power: supply: max8903: add DC and USB input current-limit GPIO controls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] Stack out-of-bounds read and endianness corruption due to `bitmap_from_arr32` with a 4-byte `u32`.
- [High] Missing upper-bound validation on `dc-current-limit-gpios` array length leads to out-of-bounds read and undefined behavior.
- [Medium] Incorrect source selection in get/set properties when DC is active but only USB has a configured GPIO.
- [Medium] Negative current limit requests bypass the minimum bounds check due to a signedness bug.
- [Low] Memory leak of temporary parsing buffer `map` due to devm_kcalloc.

Pre-existing issues:
- [High] TOCTOU data race between IRQ handlers due to prematurely dropped `source_lock`.
--

commit e36685a3d0ef54240804ea29cb948b482e9cc91e
Author: Herman van Hazendonk <github.com@herrie.org>

power: supply: max8903: add DC and USB input current-limit GPIO controls
    
This commit adds optional current-limit controls for both DC and USB power
inputs via the POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT property. It parses
device tree nodes for GPIO values and limits, then dynamically steers
requests to the currently active source (DC or USB) while introducing a
mutex to serialize source state changes.

> diff --git a/drivers/power/supply/max8903_charger.c b/drivers/power/supply/max8903_charger.c
> index 45fbaad6c6470..19c2b348a0456 100644
> --- a/drivers/power/supply/max8903_charger.c
> +++ b/drivers/power/supply/max8903_charger.c

[ ... ]

> @@ -47,6 +76,24 @@ static int max8903_get_property(struct power_supply *psy,
>  		union power_supply_propval *val)
>  {

[ ... ]

> +	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> +		if (ta_in && data->dc_current_limit_gpios)
> +			val->intval = dc_limit;
> +		else if (usb_in && data->usb_current_limit_gpio)
> +			val->intval = usb_limit;
> +		else
> +			return -ENODATA;
> +		break;

[Severity: Medium]
If both sources are active (where hardware prioritizes DC) but the device
tree only configures USB GPIOs, won't this fall through to the USB branch?

This could result in reporting the inactive USB limit while the hardware
continues drawing unmanaged power from the DC input. Should the code reject
requests for the active source if its GPIOs are missing instead of falling
back to the inactive source?

[ ... ]

> +static int max8903_set_dc_current_limit(struct max8903_data *data, u32 limit_ua)
> +{
> +	int i, best_idx = -1;
> +	u32 best_gpio_value;
> +	/*
> +	 * gpio_value is a u32 in the DT mapping and is parse-time
> +	 * validated to fit in BIT(ndescs); size the bitmap to the full
> +	 * width of the source u32 so a DT with up to 32 dc-current-limit
> +	 * GPIOs cannot overflow this stack buffer.
> +	 */
> +	DECLARE_BITMAP(values, BITS_PER_TYPE(u32));

[ ... ]

> +	best_gpio_value = data->dc_current_limit_map[best_idx].gpio_value;
> +	bitmap_from_arr32(values, &best_gpio_value, BITS_PER_TYPE(u32));

[Severity: High]
Can this cause a stack out-of-bounds read and endianness corruption?

The bitmap_from_arr32() macro casts the 4-byte pointer to best_gpio_value
to an 8-byte unsigned long pointer on 64-bit systems. This would pull
4 bytes of adjacent uninitialized stack memory into the values bitmap.

On big-endian 64-bit systems, this could incorrectly interpret the value,
placing the intended value in the upper 32 bits and garbage in the lower.

> +	gpiod_set_array_value_cansleep(data->dc_current_limit_gpios->ndescs,
> +				       data->dc_current_limit_gpios->desc,
> +				       data->dc_current_limit_gpios->info,
> +				       values);

[ ... ]

> +static int max8903_set_property(struct power_supply *psy,
> +		enum power_supply_property psp,
> +		const union power_supply_propval *val)
> +{
> +	struct max8903_data *data = power_supply_get_drvdata(psy);
> +	int ret;
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> +		/*
> +		 * Hold source_lock across the source check and the
> +		 * resulting hardware write so the IRQ handler cannot
> +		 * flip ta_in/usb_in between them and have us program the
> +		 * limit for a source that has just gone offline.
> +		 */
> +		mutex_lock(&data->source_lock);
> +		if (data->ta_in && data->dc_current_limit_gpios)
> +			ret = max8903_set_dc_current_limit(data, val->intval);
> +		else if (data->usb_in && data->usb_current_limit_gpio)
> +			ret = max8903_set_usb_current_limit(data, val->intval);
> +		else
> +			ret = -EINVAL;
> +		mutex_unlock(&data->source_lock);
> +		return ret;

[Severity: Medium]
Could a negative requested limit bypass the minimum bounds check here?

The requested limit in val->intval is a signed integer, but it is passed
to functions like max8903_set_usb_current_limit() which expect an unsigned
u32 limit_ua. If a negative value is requested, the implicit cast to u32
would create a very large unsigned value. This would bypass the upper bounds
check and silently configure the maximum possible current instead of
returning -EINVAL.

[ ... ]

> @@ -91,12 +272,21 @@ static irqreturn_t max8903_dcin(int irq, void *_data)
>  	 * library as the line should be flagged GPIO_ACTIVE_LOW in the device
>  	 * tree.
>  	 */
> +	/*
> +	 * Sample the line under source_lock so a concurrent
> +	 * max8903_set_property() observes either the old or the new
> +	 * state consistently, never a torn read where the lock is held
> +	 * but the cached flag is about to be updated.
> +	 */
> +	mutex_lock(&data->source_lock);
>  	ta_in = gpiod_get_value(data->dok);
> -
> -	if (ta_in == data->ta_in)
> +	if (ta_in == data->ta_in) {
> +		mutex_unlock(&data->source_lock);
>  		return IRQ_HANDLED;
> +	}
>  
>  	data->ta_in = ta_in;
> +	mutex_unlock(&data->source_lock);
>  
>  	/* Set Current-Limit-Mode 1:DC 0:USB */

[Severity: High]
This is a pre-existing issue, but does dropping the lock before evaluating
data->usb_in and calculating the charger enable state expose a race condition?

If DC and USB states change simultaneously, the threads could interleave.
For example, a DC unplug could evaluate the charger enable state using a
stale data->usb_in = 0, while a concurrent USB plug updates usb_in = 1.
Could this result in writing the wrong enable state to the hardware?

Should the mutex coverage be extended over the entire state evaluation block
in both max8903_dcin() and max8903_usbin()?

[ ... ]

> +static int max8903_parse_dc_current_limit(struct platform_device *pdev,
> +					  struct max8903_data *data)
> +{

[ ... ]

> +	map = devm_kcalloc(dev, map_size, sizeof(*map), GFP_KERNEL);
> +	if (!map)
> +		return -ENOMEM;

[Severity: Low]
Does this leak the temporary map buffer?

The array map is allocated using devm_kcalloc() to temporarily hold device
tree properties for parsing. Once parsed, the buffer isn't explicitly freed,
meaning the devres allocation will persist and waste memory until the
device is unbound.

Would it be better to use kcalloc() and explicitly kfree() it at the end
of the function?

[ ... ]

> +	for (i = 0; i < data->dc_current_limit_map_size; i++) {
> +		u32 gpio_value = map[i * 2 + 1];
> +
> +		/*
> +		 * gpio_value is the bitmap programmed across the
> +		 * dc-current-limit GPIOs, so it cannot represent more
> +		 * bits than the GPIO array width. A larger value would
> +		 * be silently truncated by gpiod_set_array_value() and
> +		 * select the wrong limit; reject it at parse time so
> +		 * the bogus DT is visible to the integrator.
> +		 */
> +		if (gpio_value >= BIT(data->dc_current_limit_gpios->ndescs)) {

[Severity: High]
Does this missing bounds check on ndescs lead to undefined behavior?

The ndescs value is populated from the device tree, and the stack values
array is sized to 32 bits. If the device tree provides an ndescs value of
32 or greater, the BIT(ndescs) macro would trigger undefined behavior on
32-bit systems.

Additionally, passing an unvalidated ndescs to gpiod_set_array_value_cansleep()
could read out of bounds from the values stack array. Should we ensure
ndescs is strictly less than 32 during initialization?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603071519.807604-1-github.com@herrie.org?part=2