[PATCH v3] leds: pwm: Annotate leds[] with __counted_by()

[PATCH v3] leds: pwm: Annotate leds[] with __counted_by()

[Mert Seftali]

Add the __counted_by() attribute to the flexible array member leds[] in
struct led_pwm_priv so the compiler and runtime (e.g. FORTIFY_SOURCE,
UBSAN_BOUNDS) can bounds-check accesses against num_leds.

For the annotation to be correct, num_leds must equal the number of
allocated elements before leds[] is accessed. Even taking the address
&priv->leds[i] is bounds-checked against num_leds under UBSAN_BOUNDS. The
driver allocates device_get_child_node_count() elements up front, so set
num_leds to that count right after allocation.

While here, pass the led_pwm_data element into led_pwm_add() instead of
the whole led_pwm_priv, so the helper no longer needs to index the array;
the caller walks a led_pwm_data pointer over priv->leds.

No functional change intended.

Suggested-by: Lee Jones <lee@kernel.org>
Signed-off-by: Mert Seftali <mertsftl@gmail.com>
---
Changes in v3:
- v2 followed the review suggestion to pass the element and increment
  num_leds in the caller. That is exactly what Sashiko caught: with
  __counted_by(num_leds), &priv->leds[num_leds] is bounds-checked, so
  incrementing num_leds from 0 as the loop runs trips UBSAN_BOUNDS on
  probe. num_leds is now set to the full count up front and the caller
  walks a led_pwm_data pointer, which keeps the element-passing and the
  dropped index variable, just without the running counter.

Changes in v2 (per Lee Jones review):
- Pass the led_pwm_data element into led_pwm_add() so it drops the priv and
  index arguments.

Build-tested only (no PWM-LED hardware). The __counted_by bounds behaviour
that motivated v3 was verified with a standalone UBSAN reproducer on gcc and
clang.

 drivers/leds/leds-pwm.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index 6c1f2f50ff85..d1116a5cb2b2 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -36,7 +36,7 @@ struct led_pwm_data {
 
 struct led_pwm_priv {
 	int num_leds;
-	struct led_pwm_data leds[];
+	struct led_pwm_data leds[] __counted_by(num_leds);
 };
 
 static int led_pwm_set(struct led_classdev *led_cdev,
@@ -81,10 +81,9 @@ static int led_pwm_default_brightness_get(struct fwnode_handle *fwnode,
 }
 
 __attribute__((nonnull))
-static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
+static int led_pwm_add(struct device *dev, struct led_pwm_data *led_data,
 		       struct led_pwm *led, struct fwnode_handle *fwnode)
 {
-	struct led_pwm_data *led_data = &priv->leds[priv->num_leds];
 	struct led_init_data init_data = { .fwnode = fwnode };
 	int ret;
 
@@ -167,12 +166,12 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
 		}
 	}
 
-	priv->num_leds++;
 	return 0;
 }
 
 static int led_pwm_create_fwnode(struct device *dev, struct led_pwm_priv *priv)
 {
+	struct led_pwm_data *led_data = priv->leds;
 	struct led_pwm led;
 	int ret;
 
@@ -193,7 +192,7 @@ static int led_pwm_create_fwnode(struct device *dev, struct led_pwm_priv *priv)
 
 		led.default_state = led_init_default_state_get(fwnode);
 
-		ret = led_pwm_add(dev, priv, &led, fwnode);
+		ret = led_pwm_add(dev, led_data++, &led, fwnode);
 		if (ret)
 			return ret;
 	}
@@ -217,6 +216,8 @@ static int led_pwm_probe(struct platform_device *pdev)
 	if (!priv)
 		return -ENOMEM;
 
+	priv->num_leds = count;
+
 	ret = led_pwm_create_fwnode(&pdev->dev, priv);
 
 	if (ret)
-- 
2.55.0

Re: [PATCH v3] leds: pwm: Annotate leds[] with __counted_by()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The refactoring to use pointer arithmetic (led_data++) bypasses the newly added __counted_by() bounds checking.
--

--- Patch [1]: [PATCH v3] leds: pwm: Annotate leds[] with __counted_by() ---
commit c418c024dbd5055ff67d14fc9a244c4d09795169
Author: Mert Seftali <mertsftl@gmail.com>

leds: pwm: Annotate leds[] with __counted_by()

Adds the __counted_by() attribute to the flexible array member leds[] in
struct led_pwm_priv for bounds checking. To avoid UBSAN triggers during
initialization, the code is refactored to pass a led_pwm_data pointer into
led_pwm_add() instead of indexing the array directly.

> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> --- a/drivers/leds/leds-pwm.c
> +++ b/drivers/leds/leds-pwm.c
> @@ -81,10 +81,9 @@ static int led_pwm_default_brightness_get(struct fwnode_handle *fwnode,
>  }
>  
>  __attribute__((nonnull))
> -static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
> +static int led_pwm_add(struct device *dev, struct led_pwm_data *led_data,
>  		       struct led_pwm *led, struct fwnode_handle *fwnode)
>  {
> -	struct led_pwm_data *led_data = &priv->leds[priv->num_leds];

[ ... ]

>  static int led_pwm_create_fwnode(struct device *dev, struct led_pwm_priv *priv)
>  {
> +	struct led_pwm_data *led_data = priv->leds;
>  	struct led_pwm led;
>  	int ret;
>  
> @@ -193,7 +192,7 @@ static int led_pwm_create_fwnode(struct device *dev, struct led_pwm_priv *priv)
>  
>  		led.default_state = led_init_default_state_get(fwnode);
>  
> -		ret = led_pwm_add(dev, priv, &led, fwnode);
> +		ret = led_pwm_add(dev, led_data++, &led, fwnode);

[Severity: Medium]
By decaying the flexible array to a pointer and using pointer arithmetic
(led_data++), doesn't the compiler lose the context needed to enforce the
__counted_by bounds check?

Compilers and sanitizers typically enforce __counted_by mitigations during
array subscripting operations like priv->leds[i]. Because priv->leds is
decayed into a standalone pointer and passed to led_pwm_add(), all bounds
information relative to priv->num_leds appears to be lost.

Will the primary objective of adding __counted_by be neutralized by passing
the element as a bare pointer here rather than using an index?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710070141.37214-1-mertsftl@gmail.com?part=1

Re: [PATCH v3] leds: pwm: Annotate leds[] with __counted_by()

[Mert Seftali]

Hi Lee,

Sashiko is right and I tested it too now to confirm. Under UBSAN_BOUNDS
&priv->leds[i] traps on an out-of-range index, but the led_data++ walk
in v3 does not: decaying the array to a plain led_pwm_data * drops the
__counted_by association, so num_leds is only enforced on subscript
accesses and v3 has none.

Getting the check back means &priv->leds[i] subscripting. I can still
pass that element into led_pwm_add() as you suggested, it just brings a
loop index back. So it is a tradeoff: the cleaner pointer form, or the
annotation actually enforced.

Since the element-passing was your suggestion, which would you prefer:
keep the pointer walk (annotation stays documentary), or respin with
subscripting so num_leds is enforced? Happy to do either.

Thanks,
Mert