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

["Uwe Kleine-König" <ukleinek@kernel.org>]

[-- 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 --]