Re: [PATCH v2 4/5] bcma: add host driver for the BCM6362 integrated WLAN backplane

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] bcma: infinite loop and memory corruption in block I/O accessors on unaligned count
- [Medium] bcma: driver_unregister WARN triggered by unconditional unregister on module exit
- [Medium] bcma: memory and iomap leak on bcma_bus_register error path
--

commit cab76f19668054d5036b4bcfb106104ceaeac7fc
Author: Alessio Ferri <alessio.ferri@mythread.it>

bcma: add host driver for the BCM6362 integrated WLAN backplane

The patch introduces a host driver for the BCM6362 integrated WLAN backplane.
It provides functions to power on the core, enable the clock, and implement
block I/O accessors for register reads/writes.

> diff --git a/drivers/bcma/host_soc_bcm6362.c b/drivers/bcma/host_soc_bcm6362.c
> new file mode 100644
> index 0000000000000..524f7bddd65f5
> --- /dev/null
> +++ b/drivers/bcma/host_soc_bcm6362.c

[ ... ]

> +static void bcma_host_soc_bcm6362_block_read(struct bcma_device *core,
> +					     void *buffer, size_t count,
> +					     u16 offset, u8 reg_width)
> +{
> +	void __iomem *addr = core->io_addr + offset;
> +
> +	switch (reg_width) {

[ ... ]

> +	case sizeof(u32): {
> +		__be32 *buf = buffer;
> +
> +		WARN_ON(count & 3);
> +		while (count) {
> +			*buf = (__force __be32)__raw_readl(addr);
> +			buf++;
> +			count -= 4;
> +		}
> +		break;
> +	}

[Severity: High]
If an unaligned count is passed to this function, could the count underflow?

Since WARN_ON only prints a backtrace and continues execution, a count
that isn't a multiple of 4 would bypass exactly 0 during count -= 4. This
would underflow the unsigned integer to its maximum value, causing the loop
to run almost infinitely and resulting in an out-of-bounds memory write.

[ ... ]

> +static void bcma_host_soc_bcm6362_block_write(struct bcma_device *core,
> +					      const void *buffer, size_t count,
> +					      u16 offset, u8 reg_width)
> +{
> +	void __iomem *addr = core->io_addr + offset;
> +
> +	switch (reg_width) {

[ ... ]

> +	case sizeof(u32): {
> +		const __be32 *buf = buffer;
> +
> +		WARN_ON(count & 3);
> +		while (count) {
> +			__raw_writel((__force u32)*buf, addr);
> +			buf++;
> +			count -= 4;
> +		}
> +		break;
> +	}

[Severity: High]
Similar to the read path above, could an unaligned count cause an integer
underflow here? This might lead to an infinite loop and out-of-bounds
memory reads from the buf pointer.

[ ... ]

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

[ ... ]

> +	err = bcma_bus_register(bus);
> +	if (err) {
> +		bcma_host_soc_bcm6362_power_off(host);
> +		return err;
> +	}
> +
> +	platform_set_drvdata(pdev, host);
> +
> +	return 0;
> +}

[Severity: Medium]
If bcma_bus_register() fails (for example, if bcma_bus_scan() encounters
a corrupted EROM entry and returns -EILSEQ), does this error path leak any
cores successfully scanned prior to the error?

It appears the dynamically allocated core structures in the bus->cores list
and their associated ioremap() mappings are abandoned. Should this call
bcma_unregister_cores(bus) to tear them down?

> diff --git a/drivers/bcma/main.c b/drivers/bcma/main.c
> index 72f045e6ed513..4ccc9fb98f616 100644
> --- a/drivers/bcma/main.c
> +++ b/drivers/bcma/main.c

[ ... ]

> @@ -682,6 +682,11 @@ static int __init bcma_modinit(void)
>  		pr_err("SoC host initialization failed\n");
>  		err = 0;
>  	}
> +	err = bcma_host_soc_bcm6362_register_driver();
> +	if (err) {
> +		pr_err("BCM6362 WLAN host initialization failed\n");
> +		err = 0;
> +	}

[Severity: Medium]
Does swallowing the registration error here cause problems during module
unload?

By zeroing out err, the module successfully loads even if registration
fails. Later, bcma_modexit() unconditionally unregisters the driver:

>  static void __exit bcma_modexit(void)
>  {
>  #ifdef CONFIG_BCMA_HOST_PCI
>  	bcma_host_pci_exit();
>  #endif
> +	bcma_host_soc_bcm6362_unregister_driver();
>  	bcma_host_soc_unregister_driver();
>  	bus_unregister(&bcma_bus_type);
>  }

Calling platform_driver_unregister() on a driver that failed to register
passes a NULL pointer to the core driver_unregister() function, which
directly triggers an unexpected driver unregister WARN splat. Could the
registration status be tracked, or errors propagated instead of being ignored?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-bcm6362-wlan-v2-0-4752da7e0982@mythread.it?part=4