Re: [PATCH v7 2/4] pwm: sun8i: Add H616 PWM support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] Insufficient memory is allocated for the `parent_names` array, causing a heap buffer overflow when the clock has multiple parents.
- [High] The driver mutates statically allocated global clock templates during probe, causing MMIO address corruption and Use-After-Free.
- [High] Computed PWM duty cycle suffers from integer truncation, resulting in a 0% duty cycle when 100% is requested at the maximum period.
- [High] Composite clocks manually registered during probe are leaked on error paths.
- [High] The driver registers clocks with the CCF before deasserting the hardware reset control, risking MMIO accesses to a reset block.
- [High] Error in `sun8i_pwm_request` leaks channel state, and `sun8i_pwm_free` has a data race.
- [High] Direct hardware modification inside the clock framework's `of_xlate` callback.
- [High] Missing error check for `clk_round_rate()` return value assigned to an unsigned variable.
- [High] Shared parent clock rate can be arbitrarily modified by sibling channels, disrupting active waveforms.
--

commit 0552ac994a2720884880e2f7a90a1ceb9b7e36e6
Author: Richard Genoud <richard.genoud@bootlin.com>

pwm: sun8i: Add H616 PWM support
    
Add driver for Allwinner H616 PWM controller, supporting up to 6
channels.
Those channels output can be either a PWM signal output or a clock
output, thanks to the bypass.

[ ... ]

> diff --git a/drivers/pwm/pwm-sun8i.c b/drivers/pwm/pwm-sun8i.c
> new file mode 100644
> index 0000000000000..8f1023e3a2e5a
> --- /dev/null
> +++ b/drivers/pwm/pwm-sun8i.c
> @@ -0,0 +1,938 @@

[ ... ]

> +static int sun8i_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct sun8i_pwm_chip *sun8i_chip = sun8i_pwm_from_chip(chip);
> +	struct sun8i_pwm_channel *chan = &sun8i_chip->channels[pwm->hwpwm];
> +
> +	scoped_guard(spinlock_irqsave, &sun8i_chip->clk_pdata->lock) {
> +		if (chan->mode == SUN8I_PWM_MODE_CLK)
> +			return -EBUSY;
> +		chan->mode = SUN8I_PWM_MODE_PWM;
> +	}
> +
> +	return clk_prepare_enable(chan->pwm_clk);
> +}

[Severity: High]
If clk_prepare_enable() fails and returns an error, does this leave the
channel permanently marked as SUN8I_PWM_MODE_PWM? This could prevent future
use of the channel.

> +static void sun8i_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct sun8i_pwm_chip *sun8i_chip = sun8i_pwm_from_chip(chip);
> +	struct sun8i_pwm_channel *chan = &sun8i_chip->channels[pwm->hwpwm];
> +
> +	clk_disable_unprepare(chan->pwm_clk);
> +	chan->mode = SUN8I_PWM_MODE_NONE;
> +}

[Severity: High]
Is there a data race here?

The chan->mode state is modified without holding clk_pdata->lock, but it is 
read and written concurrently under the lock in sun8i_pwm_request() and 
sun8i_pwm_of_clk_get().

[ ... ]

> +static int sun8i_pwm_write_waveform(struct pwm_chip *chip,
> +				    struct pwm_device *pwm, const void *_wfhw)
> +{
> +	const struct sun8i_pwm_waveform *wfhw = _wfhw;
> +	struct sun8i_pwm_chip *sun8i_chip = sun8i_pwm_from_chip(chip);
> +	struct sun8i_pwm_channel *chan = &sun8i_chip->channels[pwm->hwpwm];
> +	unsigned long flags;
> +	u32 val;
> +	int ret;
> +
> +	ret = clk_set_rate(chan->pwm_clk, wfhw->clk_rate);
> +	if (ret)
> +		return ret;

[Severity: High]
Since the channel clocks are registered with the CLK_SET_RATE_PARENT flag,
they share paired parent clocks. If clk_set_rate() changes the frequency of
the shared parent to satisfy this request, will it transparently corrupt the
active PWM waveform output of the sibling channel?

[ ... ]

> +static int sun8i_pwm_round_waveform_tohw(struct pwm_chip *chip,
> +					 struct pwm_device *pwm,
> +					 const struct pwm_waveform *wf,
> +					 void *_wfhw)
> +{

[ ... ]

> +	max_rate = clk_round_rate(chan->pwm_clk, U32_MAX);

[Severity: High]
Since clk_round_rate() returns a long that can contain a negative error
code, will assigning it directly to the unsigned long max_rate cause issues? 

If an error is returned, this would cast to a massive unsigned value and
corrupt the subsequent period_ratio calculations.

[ ... ]

> +	wfhw->duty_ticks = mul_u64_u64_div_u64(calc_rate, duty_cycle,
> +					       NSEC_PER_SEC);
> +
> +	if (wfhw->duty_ticks > wfhw->period_ticks)
> +		wfhw->duty_ticks = wfhw->period_ticks;

[Severity: High]
Does this calculation suffer from integer truncation?

The wfhw->duty_ticks field is declared as a u16. If the period is at its
maximum (65536) and a 100% duty cycle is requested, the 64-bit calculation
yields 65536. When assigned to the u16 field, this will silently truncate
to 0.

The subsequent bounds check:
    if (wfhw->duty_ticks > wfhw->period_ticks)

would evaluate as 0 > 65536, which is false, resulting in the hardware
being programmed with a 0% duty cycle instead of 100%.

[ ... ]

> +static struct clk_hw *sun8i_pwm_of_clk_get(struct of_phandle_args *clkspec,
> +					   void *data)
> +{

[ ... ]

> +	if (IS_ERR(ret_clk))
> +		goto out;
> +
> +	sun8i_pwm_set_bypass(sun8i_chip, idx, true);
> +out:
> +	return ret_clk;
> +}

[Severity: High]
Is it safe to directly mutate hardware registers via sun8i_pwm_set_bypass()
inside the clock framework's translation callback?

Clock translation can happen speculatively or multiple times (for example,
when a client driver defers probe). Mutating hardware state during
translation bypasses standard clock lifecycle semantics (prepare/enable)
and might leave the hardware in an unexpected state if the translation
doesn't proceed to activation.

[ ... ]

> +static int sun8i_add_composite_clk(struct clk_pwm_data *data,
> +				   void __iomem *reg, spinlock_t *lock,
> +				   struct device *dev, struct clk_hw **hw)
> +{
> +	const struct clk_ops *mux_ops = NULL, *gate_ops = NULL, *rate_ops = NULL;
> +	struct clk_hw *mux_hw = NULL, *gate_hw = NULL, *rate_hw = NULL;
> +	struct device_node *node = dev->of_node;
> +
> +	if (data->mux_hw) {
> +		struct clk_mux *mux;
> +
> +		mux_hw = data->mux_hw;
> +		mux = to_clk_mux(mux_hw);
> +		mux->lock = lock;
> +		mux_ops = mux_hw->init->ops;
> +		mux->reg = (uintptr_t)mux->reg + reg;
> +	}

[Severity: High]
Does this code mutate statically allocated global clock templates during probe?

The pwmcc_data array and its referenced clock component objects (like
mux_xy_01) are globally static. Modifying these instances with per-device
state (such as adding the current MMIO base via mux->reg) means if the
driver probes multiple instances, or unbinds and rebounds, the global MMIO
pointers will be offset multiple times, leading to bad memory accesses.

[ ... ]

> +	/*
> +	 * Retrieve the parent clock names from DTS for pwm-clk-srcxy
> +	 */
> +	if (!data->parent_names) {
> +		data->num_parents = of_clk_get_parent_count(node);
> +		if (data->num_parents == 0)
> +			return -ENOENT;
> +
> +		data->parent_names = devm_kzalloc(dev,
> +						  sizeof(*data->parent_names),
> +						  GFP_KERNEL);
> +		for (unsigned int i = 0; i < data->num_parents; i++)
> +			data->parent_names[i] = of_clk_get_parent_name(node, i);
> +	}

[Severity: High]
Is there a heap buffer overflow here if the device tree specifies multiple
clocks?

The allocation size is sizeof(*data->parent_names), which evaluates to the
size of a single pointer (8 bytes on 64-bit systems). However, the loop
writes data->num_parents pointers into this allocation. 

Also, as mentioned above, modifying data->parent_names mutates the static
global template. If the device unbinds and rebounds, data->parent_names
will hold a dangling pointer to freed devm memory, leading to a
use-after-free on the next probe.

[ ... ]

> +static int sun8i_pwm_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	ret = sun8i_pwm_init_clocks(pdev, sun8i_chip);
> +	if (ret)
> +		return ret;
> +
> +	for (unsigned int i = 0; i < data->npwm; i++) {
> +		struct sun8i_pwm_channel *chan = &sun8i_chip->channels[i];
> +		struct clk_hw **hw = &sun8i_chip->clk_pdata->hw_data->hws[i];
> +
> +		chan->pwm_clk = devm_clk_hw_get_clk(dev, *hw, NULL);
> +		if (IS_ERR(chan->pwm_clk)) {
> +			ret = dev_err_probe(dev, PTR_ERR(chan->pwm_clk),
> +					    "Failed to register PWM clock %d\n", i);
> +			return ret;
> +		}
> +		chan->mode = SUN8I_PWM_MODE_NONE;
> +	}
> +
> +	ret = devm_of_clk_add_hw_provider(dev, sun8i_pwm_of_clk_get, sun8i_chip);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add HW clock provider\n");
> +
> +	ret = devm_add_action_or_reset(dev, sun8i_pwm_unregister_clk,
> +				       sun8i_chip->clk_pdata->hw_data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add devm action\n");

[Severity: High]
Are the composite clocks leaked on early return paths?

sun8i_pwm_init_clocks() manually registers clock objects with the CCF, but
the teardown action (sun8i_pwm_unregister_clk) is only hooked up to devres
much later. If an error occurs in the interim (like devm_clk_hw_get_clk()
or devm_of_clk_add_hw_provider() failing), the probe routine returns early
and the registered clocks are permanently leaked into the CCF.

> +	/* Deassert reset */
> +	sun8i_chip->rst = devm_reset_control_get_shared_deasserted(dev, NULL);
> +	if (IS_ERR(sun8i_chip->rst))
> +		return dev_err_probe(dev, PTR_ERR(sun8i_chip->rst),
> +				     "Failed to get reset control\n");

[Severity: High]
Is it safe to register the clocks with the CCF before deasserting the
hardware reset control?

The CCF may read hardware registers immediately upon registration to query
clock parents or rates. Accessing a peripheral's MMIO space while it is
held in reset can cause synchronous external aborts, bus stalls, or kernel
panics.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703152215.192859-1-richard.genoud@bootlin.com?part=2