Thank you for your contribution! Sashiko AI review found 20 potential issue(s) to consider: - [High] mm: memory leak of adc->buffer on teardown - [High] stm32-mdf: race condition in MDF core startup and clock locking - [High] iio: adc: heap overflow in adc->channels allocation - [High] iio: adc: triggered buffer resources leaked on removal - [High] iio: adc: misaligned cyclic DMA buffer size causes OOB read - [High] iio: adc: unbalanced iio_device_unregister on error path - [High] stm32-mdf: UAF in IRQ from unprotected list_for_each_entry - [High] stm32-mdf: dangling sitf pointer on unbind (Use-After-Free) - [High] stm32-mdf-core: lockless RMW corrupts hardware state in clk_cckx_gate_endisable - [High] iio: adc: TOCTOU race on adc->cb causes NULL pointer deref - [High] stm32-mdf: interleaved SITFs fail to start - [Medium] of: resource leak of fwnode references - [Medium] iio: adc: uninitialized channels exposed to subsystem - [Medium] stm32-mdf-serial: clock reference leak on unbind - [Medium] iio: adc: off-by-one error leaks first backend on error path - [Medium] iio: adc: 32-bit overflow when computing settling time discard count - [Medium] stm32-mdf-core: rigid gcd() restricts cckdiv to powers of 2 - [Medium] stm32-mdf-core: clk_rate_exclusive_get error ignored - [Low] kconfig: selecting non-existent COMMON_CLK_REGMAP - [Low] pm: missing pm_ptr() macro wrapper causes dead code --
commit b396898f85019ba218650a01b961b3f92e6b164b Author: Olivier Moysan <[email protected]> Subject: iio: adc: add stm32 mdf support This commit introduces the STM32 Multi-function Digital Filter (MDF) core, serial interface, and digital filter drivers. It supports converting bitstreams from sigma-delta modulators or digital microphones into samples via IIO. > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig [ ... ] > +config STM32_MDF_ADC > + tristate "STMicroelectronics STM32 MDF adc" > + depends on (ARCH_STM32 && OF) || COMPILE_TEST > + select STM32_MDF_CORE > + select COMMON_CLK_REGMAP [Severity: Low] This isn't a bug, but COMMON_CLK_REGMAP does not appear to exist in the kernel tree. Does selecting an undeclared config symbol cause unmet dependency issues during build? > diff --git a/drivers/iio/adc/stm32-mdf-adc.c b/drivers/iio/adc/stm32-mdf-adc.c [ ... ] > +static int mdf_adc_set_samp_freq(struct iio_dev *indio_dev) > +{ [ ... ] > + adc->nbdis = DIV_ROUND_UP(adc->stu * sample_freq, 1000000); [Severity: Medium] Could this multiplication overflow on 32-bit architectures? Both variables are 32-bit types, so a large settling time (e.g., 100ms) and sample frequency (48kHz) might exceed U32_MAX before the division. [ ... ] > +static int stm32_mdf_adc_start_conv(struct iio_dev *indio_dev) > +{ > + struct stm32_mdf_adc *adc = iio_priv(indio_dev); > + int ret; > + > + ret = stm32_mdf_sitf_start(adc->sitf); > + if (ret < 0) > + return ret; > + > + ret = stm32_mdf_adc_start_filter(adc); [Severity: High] Are the serial interfaces for interleaved instances omitted here? The start process cascades to interleaved filters via stm32_mdf_adc_start_filter(), but it seems we only ever call stm32_mdf_sitf_start() on the primary adc->sitf. [ ... ] > +static void stm32_mdf_adc_dma_buffer_done(void *data) > +{ > + struct iio_dev *indio_dev = data; > + struct stm32_mdf_adc *adc = iio_priv(indio_dev); > + int available = stm32_mdf_adc_dma_residue(adc); > + size_t old_pos; [ ... ] > + while (available >= indio_dev->scan_bytes) { > + s32 *buffer = (s32 *)&adc->rx_buf[adc->bufi]; > + > + adc->bufi += indio_dev->scan_bytes; > + if (adc->bufi >= adc->buf_sz) { [Severity: High] Is it possible for adc->bufi to overshoot adc->buf_sz without hitting it exactly? Since adc->buf_sz in stm32_mdf_set_watermark() is calculated based on sizeof(u32), if scan_bytes is not a multiple of sizeof(u32), this loop might read out of bounds of the physical allocation before wrapping around. > + if (adc->cb) > + adc->cb(&adc->rx_buf[old_pos], adc->buf_sz - > old_pos, adc->cb_priv); [Severity: High] Is this vulnerable to a race condition? If an external consumer concurrently calls stm32_mdf_release_buff_cb() and clears adc->cb and adc->cb_priv without locks, the callback might be evaluated as non-NULL but executed as NULL, or executed with a NULL cb_priv. [ ... ] > +static irqreturn_t stm32_mdf_irq(int irq, void *arg) > +{ [ ... ] > + if (flags & MDF_DFLTISR_FTHF_MASK) { > + /* Reading the data register clear the IRQ status */ > + regmap_read(regmap, MDF_DFLTDR_REG, ptr++); > + > + if (MDF_IS_FILTER0(adc)) > + list_for_each_entry(adc_inter, &adc->mdf->filter_list, > entry) [Severity: High] Could this list traversal crash the kernel if the list is mutated concurrently? The interrupt handler iterates adc->mdf->filter_list while stm32_mdf_adc_remove() performs list_del(&adc->entry) without locks or RCU primitives. [ ... ] > +static int stm32_mdf_get_sitf(struct device *dev, struct stm32_mdf_adc *adc, > + struct fwnode_handle *sitf_node) > +{ > + struct stm32_mdf_sitf *sitf; > + > + /* Look for sitf interface from node handle */ > + list_for_each_entry(sitf, &adc->mdf->sitf_list, entry) { > + if (sitf->node == sitf_node) { > + adc->sitf = sitf; > + break; > + } > + } [Severity: High] Is there a risk of a Use-After-Free if the sibling sitf device is unbound? The ADC driver stores a raw pointer to sitf (allocated via devm_kzalloc by the sibling device) without establishing a device link or reference count. [ ... ] > +static int stm32_mdf_adc_postenable(struct iio_dev *indio_dev) > +{ [ ... ] > + while (adc->channels[i].backend && i < indio_dev->num_channels) { > + ret = iio_backend_enable(adc->channels[i].backend); > + if (ret < 0) { > + while (--i > 0) > + iio_backend_disable(adc->channels[i].backend); [Severity: Medium] Does this loop completely miss disabling the backend at index 0? If iio_backend_enable() fails when i = 1, the decrement evaluates to 0, stopping the loop before it can disable adc->channels[0].backend. [ ... ] > +static int stm32_mdf_audio_init(struct device *dev, struct iio_dev > *indio_dev) > +{ > + struct stm32_mdf_adc *adc = iio_priv(indio_dev); > + struct iio_chan_spec *ch; > + int ret; > + > + ch = devm_kzalloc(&indio_dev->dev, sizeof(*ch), GFP_KERNEL); > + if (!ch) > + return -ENOMEM; > + > + adc->channels = devm_kzalloc(&indio_dev->dev, sizeof(*adc->channels), > GFP_KERNEL); > + if (!adc->channels) > + return -ENOMEM; > + > + ret = stm32_mdf_adc_chan_init(indio_dev, ch); [Severity: High] Can this cause a heap overflow? The ch and adc->channels arrays are allocated with exactly one element, but stm32_mdf_adc_chan_init() iterates over all child nodes lacking a "compatible" string. If more than one such node exists in the device tree, it will write out of bounds. [ ... ] > +static int stm32_mdf_adc_init(struct device *dev, struct iio_dev *indio_dev) > +{ [ ... ] > + indio_dev->num_channels = num_ch; > + indio_dev->channels = ch; [Severity: Medium] Are we exposing uninitialized trailing channels here? While num_ch is the total count of child nodes, stm32_mdf_adc_chan_init() skips nodes containing a "compatible" string, resulting in fewer initialized channels than num_ch. [ ... ] > + ret = iio_triggered_buffer_setup(indio_dev, > + &iio_pollfunc_store_time, NULL, > + &stm32_mdf_buffer_setup_ops); [Severity: High] Does this leak the triggered buffer and pollfunc on unbind? The manual setup here lacks a corresponding iio_triggered_buffer_cleanup() call in stm32_mdf_adc_remove(). Using devm_iio_triggered_buffer_setup() might resolve this. [ ... ] > +static int stm32_mdf_adc_parse_of(struct platform_device *pdev, struct > stm32_mdf_adc *adc) > +{ [ ... ] > + if (device_property_present(&pdev->dev, "st,sync")) { > + filt_node = fwnode_find_reference(dev_fwnode(dev), "st,sync", > 0); [ ... ] > + adc->buffer = kcalloc(buf_size, sizeof(u32), GFP_KERNEL); > + if (!adc->buffer) > + return -ENOMEM; > + > + /* Retrieve serial interface */ > + ret = fwnode_property_get_reference_args(dev_fwnode(dev), "st,sitf", > NULL, 1, 0, &args); [Severity: High] Are the memory for adc->buffer and the fwnode references leaked on teardown? The buffer is allocated with a non-managed kcalloc and never released in stm32_mdf_adc_remove(). [Severity: Medium] Additionally, both fwnode_find_reference() and fwnode_property_get_reference_args() increment the node reference counts, but they appear to be discarded without a corresponding fwnode_handle_put() anywhere. [ ... ] > +static int stm32_mdf_adc_probe(struct platform_device *pdev) > +{ [ ... ] > + if (dev_data->type == STM32_MDF_AUDIO) { > + ret = of_platform_populate(node, NULL, NULL, dev); > + if (ret < 0) { > + dev_err_probe(dev, ret, "Failed to find an audio > DAI\n"); > + goto err_unregister; > + } > + } > + > + return 0; > + > +err_unregister: > + iio_device_unregister(iio); [Severity: High] Could this lead to an unbalanced unregister call? Interleaved filters (other than 0) intentionally bypass iio_device_register(), so jumping to err_unregister here would call iio_device_unregister() on a device that was never registered. [ ... ] > +static struct platform_driver stm32_mdf_adc_driver = { > + .driver = { > + .name = "stm32-mdf-adc", > + .of_match_table = stm32_mdf_adc_match, > + .pm = &stm32_mdf_adc_pm_ops, > + }, [Severity: Low] Should the PM ops be wrapped with the pm_ptr() macro? Unconditional assignment leads to the PM structure and callbacks becoming dead code when CONFIG_PM is disabled. This pattern is also present in stm32_mdf_driver and stm32_mdf_sitf_driver. > diff --git a/drivers/iio/adc/stm32-mdf-core.c > b/drivers/iio/adc/stm32-mdf-core.c [ ... ] > +int stm32_mdf_core_start_mdf(struct stm32_mdf *mdf) > +{ > + struct stm32_mdf_priv *priv = to_stm32_mdf_priv(mdf); > + struct device *dev = &priv->pdev->dev; > + int ret; > + u32 val; > + > + if (atomic_inc_return(&priv->n_active_ch) == 1) { > + ret = pm_runtime_resume_and_get(dev); [Severity: High] Is this vulnerable to a race condition? If there are concurrent callers, the first one enters the block to power on the hardware, but the second one skips the block (since the atomic counter is > 1) and will return immediately, potentially accessing powered-off hardware before the first caller finishes. [ ... ] > + if (!priv->exclusive_flg) { > + clk_rate_exclusive_get(priv->kclk); > + priv->exclusive_flg = true; > + } [Severity: Medium] Should the return value of clk_rate_exclusive_get() be checked? If the lock fails but priv->exclusive_flg is set to true, a subsequent clk_rate_exclusive_put() could lead to an unbalanced clock rate lock. [ ... ] > +int stm32_mdf_core_stop_mdf(struct stm32_mdf *mdf) > +{ > + struct stm32_mdf_priv *priv = to_stm32_mdf_priv(mdf); > + int ret = 0; > + > + if (atomic_dec_and_test(&priv->n_active_ch)) { [Severity: High] Similar to the start function, does this need locking to prevent racing with concurrent starts? [ ... ] > +static int stm32_mdf_core_cck_divider_set_rate(struct platform_device *pdev, > + struct stm32_mdf_priv *priv, > + unsigned long parent_rate) > +{ [ ... ] > + cckdiv = gcd(ratio, MDF_CKG_CCKDIV_MAX); > + procdiv = ratio / cckdiv; [Severity: Medium] Because MDF_CKG_CCKDIV_MAX (16) is a power of 2, doesn't gcd(ratio, 16) erroneously restrict the resulting cckdiv strictly to powers of 2 (1, 2, 4, 8, 16)? If a valid clock configuration actually required cckdiv to be 9, this would compute 1 instead and could unnecessarily overflow procdiv. [ ... ] > +static void clk_cckx_gate_endisable(struct clk_hw *hw, int enable) > +{ > + struct clk_cckx_gate *gate = to_clk_cckx_gate(hw); > + unsigned long flags = 0; > + u32 reg; > + > + if (gate->lock) > + spin_lock_irqsave(gate->lock, flags); > + else > + __acquire(gate->lock); > + > + reg = clk_cckx_gate_readl(gate); > + > + if (enable) > + reg |= BIT(gate->bit_idx); > + else > + reg &= ~BIT(gate->bit_idx); > + > + clk_cckx_gate_writel(gate, reg); [Severity: High] Is gate->lock ever initialized? Since clk_cck0 and clk_cck1 statically zero-initialize their contents, gate->lock is NULL and this falls back to the empty __acquire annotation. This causes a non-atomic read-modify-write on MDF_CKGCR_REG that bypasses regmap's internal locking and will corrupt concurrent updates from regmap_set_bits(). [ ... ] > +static int stm32_mdf_core_parse_of(struct platform_device *pdev, struct > stm32_mdf_priv *priv) > +{ [ ... ] > + for (i = 0; i < count; i++) { > + handle = fwnode_find_reference(fwnode, "st,interleave", > i); > + if (IS_ERR(handle)) { > + dev_err(dev, "Failed to read filter handle: > %ld\n", > + PTR_ERR(handle)); > + return PTR_ERR(handle); > + } > + priv->mdf.fh_interleave[i] = handle; > + } [Severity: Medium] These fwnode references are stored in the fh_interleave array, but they don't seem to be released with fwnode_handle_put() anywhere on the teardown path, which leads to reference leaks. > diff --git a/drivers/iio/adc/stm32-mdf-serial.c > b/drivers/iio/adc/stm32-mdf-serial.c [ ... ] > +static int stm32_mdf_sitf_get_clk(struct device *dev, struct stm32_mdf_sitf > *sitf) > +{ > + struct clk *sck; > + > + /* Optional clock. Clock not needed in Manchester mode */ > + sck = clk_get_optional(sitf->dev, 0); [Severity: Medium] Does this leak the clock reference on unbind? The reference is acquired manually with clk_get_optional() but there is no corresponding clk_put() in stm32_mdf_sitf_remove(). Switching to devm_clk_get_optional() could prevent this. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
