[PATCH v2] pwm: lp3943: validate DT output indices in lp3943_pwm_parse_dt()

[PATCH v2] pwm: lp3943: validate DT output indices in lp3943_pwm_parse_dt()

[Manush Prajwal]

The ti,pwm0/ti,pwm1 devicetree properties are raw u32 arrays naming
which of the 16 physical LP3943 outputs (0-15, per the documented
binding and the 16-entry enum lp3943_pwm_output) each PWM channel
drives. lp3943_pwm_parse_dt() reads these values straight from the
devicetree with of_property_read_u32_array() and stores them in
pwm_map->output[] without checking they are actually in range.

Both consumers of pwm_map->output[] then use an out-of-range value
directly as an index/bit-offset with no bounds check of their own:

 - lp3943_pwm_request_map() does
       test_and_set_bit(offset, &lp3943->pin_used)
   with offset taken straight from output[i], corrupting memory beyond
   the single 'unsigned long pin_used' field for offset >= BITS_PER_LONG.

 - lp3943_pwm_set_mode() does
       mux[index].reg / .mask / .shift
   with index taken straight from output[i], reading past the 16-entry
   lp3943_mux_cfg[] array and then issuing a regmap write built from
   whatever garbage register/mask/shift values were read out of bounds.

A devicetree (malformed board file or a loaded overlay) with an
out-of-range ti,pwm0/ti,pwm1 value is enough to reach either path;
nothing upstream of these two functions validates the value.

Add a range check in lp3943_pwm_parse_dt() right after the values are
read, rejecting the devicetree with -EINVAL before any out-of-range
value can reach pwm_map->output[]. Check explicitly for negative
values in addition to the upper bound: enum lp3943_pwm_output's
underlying integer representation is implementation-defined per the C
standard, so do not rely on the compiler having chosen an unsigned
type for it even though it always does in practice for GCC (verified
empirically, with and without -fshort-enums, since none of the
enumerators are negative). Add the LP3943_NUM_OUTPUTS(16) constant
next to the existing enum it bounds, alongside the existing
LP3943_NUM_PWMS(2) constant.

Signed-off-by: Manush Prajwal <manushprajwal555@gmail.com>
---
Changes since v1:
 - Sashiko AI review flagged that the bounds check could theoretically
   be bypassed by a devicetree value with the MSB set, if the compiler
   chose a signed underlying type for enum lp3943_pwm_output. I checked
   this empirically with gcc 13.2.0, both with and without
   -fshort-enums: since none of the enum's values are negative, gcc
   always picks an unsigned underlying type, so output[j] >= 16 already
   rejects such values today. Added the explicit "< 0" check anyway,
   since the C standard leaves the enum's underlying type
   implementation-defined and the check costs nothing.
 - The same review also flagged a missing NULL check on
   pdata->pwms[hwpwm] in lp3943_pwm_request_map() as a pre-existing,
   separate bug (not introduced by this patch, confirmed by
   inspection) - sending that as its own follow-up patch rather than
   folding an unrelated fix in here.

 drivers/pwm/pwm-lp3943.c   | 7 ++++++-
 include/linux/mfd/lp3943.h | 2 ++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-lp3943.c b/drivers/pwm/pwm-lp3943.c
index 10537e74b..e8bb8dbb9 100644
--- a/drivers/pwm/pwm-lp3943.c
+++ b/drivers/pwm/pwm-lp3943.c
@@ -218,7 +218,7 @@ static int lp3943_pwm_parse_dt(struct device *dev,
 	struct lp3943_platform_data *pdata;
 	struct lp3943_pwm_map *pwm_map;
 	enum lp3943_pwm_output *output;
-	int i, err, num_outputs, count = 0;
+	int i, j, err, num_outputs, count = 0;
 
 	if (!node)
 		return -EINVAL;
@@ -247,6 +247,11 @@ static int lp3943_pwm_parse_dt(struct device *dev,
 		if (err)
 			return err;
 
+		for (j = 0; j < num_outputs; j++) {
+			if (output[j] < 0 || output[j] >= LP3943_NUM_OUTPUTS)
+				return -EINVAL;
+		}
+
 		pwm_map = devm_kzalloc(dev, sizeof(*pwm_map), GFP_KERNEL);
 		if (!pwm_map)
 			return -ENOMEM;
diff --git a/include/linux/mfd/lp3943.h b/include/linux/mfd/lp3943.h
index 5d2d172d3..844938ca7 100644
--- a/include/linux/mfd/lp3943.h
+++ b/include/linux/mfd/lp3943.h
@@ -53,6 +53,8 @@ enum lp3943_pwm_output {
 	LP3943_PWM_OUT15,
 };
 
+#define LP3943_NUM_OUTPUTS		16
+
 /*
  * struct lp3943_pwm_map
  * @output: Output pins which are mapped to each PWM channel
-- 
2.46.2.windows.1

Re: [PATCH v2] pwm: lp3943: validate DT output indices in lp3943_pwm_parse_dt()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4293 bytes --]

On Sun, Sep 06, 2026 at 05:10:07PM +0530, Manush Prajwal wrote:
> The ti,pwm0/ti,pwm1 devicetree properties are raw u32 arrays naming
> which of the 16 physical LP3943 outputs (0-15, per the documented
> binding and the 16-entry enum lp3943_pwm_output) each PWM channel
> drives. lp3943_pwm_parse_dt() reads these values straight from the
> devicetree with of_property_read_u32_array() and stores them in
> pwm_map->output[] without checking they are actually in range.
> 
> Both consumers of pwm_map->output[] then use an out-of-range value
> directly as an index/bit-offset with no bounds check of their own:
> 
>  - lp3943_pwm_request_map() does
>        test_and_set_bit(offset, &lp3943->pin_used)
>    with offset taken straight from output[i], corrupting memory beyond
>    the single 'unsigned long pin_used' field for offset >= BITS_PER_LONG.
> 
>  - lp3943_pwm_set_mode() does
>        mux[index].reg / .mask / .shift
>    with index taken straight from output[i], reading past the 16-entry
>    lp3943_mux_cfg[] array and then issuing a regmap write built from
>    whatever garbage register/mask/shift values were read out of bounds.
> 
> A devicetree (malformed board file or a loaded overlay) with an
> out-of-range ti,pwm0/ti,pwm1 value is enough to reach either path;
> nothing upstream of these two functions validates the value.
> 
> Add a range check in lp3943_pwm_parse_dt() right after the values are
> read, rejecting the devicetree with -EINVAL before any out-of-range
> value can reach pwm_map->output[]. Check explicitly for negative
> values in addition to the upper bound: enum lp3943_pwm_output's
> underlying integer representation is implementation-defined per the C
> standard, so do not rely on the compiler having chosen an unsigned
> type for it even though it always does in practice for GCC (verified
> empirically, with and without -fshort-enums, since none of the
> enumerators are negative). Add the LP3943_NUM_OUTPUTS(16) constant
> next to the existing enum it bounds, alongside the existing
> LP3943_NUM_PWMS(2) constant.
> 
> Signed-off-by: Manush Prajwal <manushprajwal555@gmail.com>
> ---
> Changes since v1:
>  - Sashiko AI review flagged that the bounds check could theoretically
>    be bypassed by a devicetree value with the MSB set, if the compiler
>    chose a signed underlying type for enum lp3943_pwm_output. I checked
>    this empirically with gcc 13.2.0, both with and without
>    -fshort-enums: since none of the enum's values are negative, gcc
>    always picks an unsigned underlying type, so output[j] >= 16 already
>    rejects such values today. Added the explicit "< 0" check anyway,
>    since the C standard leaves the enum's underlying type
>    implementation-defined and the check costs nothing.
>  - The same review also flagged a missing NULL check on
>    pdata->pwms[hwpwm] in lp3943_pwm_request_map() as a pre-existing,
>    separate bug (not introduced by this patch, confirmed by
>    inspection) - sending that as its own follow-up patch rather than
>    folding an unrelated fix in here.
> 
>  drivers/pwm/pwm-lp3943.c   | 7 ++++++-
>  include/linux/mfd/lp3943.h | 2 ++
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-lp3943.c b/drivers/pwm/pwm-lp3943.c
> index 10537e74b..e8bb8dbb9 100644
> --- a/drivers/pwm/pwm-lp3943.c
> +++ b/drivers/pwm/pwm-lp3943.c
> @@ -218,7 +218,7 @@ static int lp3943_pwm_parse_dt(struct device *dev,
>  	struct lp3943_platform_data *pdata;
>  	struct lp3943_pwm_map *pwm_map;
>  	enum lp3943_pwm_output *output;
> -	int i, err, num_outputs, count = 0;
> +	int i, j, err, num_outputs, count = 0;
>  
>  	if (!node)
>  		return -EINVAL;
> @@ -247,6 +247,11 @@ static int lp3943_pwm_parse_dt(struct device *dev,
>  		if (err)
>  			return err;
>  
> +		for (j = 0; j < num_outputs; j++) {
> +			if (output[j] < 0 || output[j] >= LP3943_NUM_OUTPUTS)
> +				return -EINVAL;
> +		}
> +

A few lines above we have:

                num_outputs = of_property_count_u32_elems(node, name[i]);
                if (num_outputs <= 0)
                        continue;

I wonder if it makes sense to clamp num_outputs also using
LP3943_NUM_OUTPUTS?!

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]