Re: [PATCH 2/2] irqchip: introduce TC9564 SoC MSIGEN support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] irqchip/tc9564: uninitialized stack variables info and dgc_info
- [High] irqchip/tc9564-msigen: Use-After-Free in handler during module removal
- [Low] kconfig: TC9564_MSIGEN depends on hidden symbol GENERIC_IRQ_CHIP
--

commit 3dfc2bc2208f270b240c50f9b867c34c54af58e0
Author: Daniel Thompson <daniel@riscstar.com>

irqchip: introduce TC9564 SoC MSIGEN support
    
Define a new irqchip platform driver that represents the MSIGEN
functionality present in the TC9564 SoC.

> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index a8f8c423b7957..205b5a167c805 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
[ ... ]
> +config TC9564_MSIGEN
> +	tristate "Toshiba TC9564 MSI distributor"
> +	depends on TC9564_PCI
> +	depends on GENERIC_IRQ_CHIP

[Severity: Low]
Is it intentional to use "depends on" instead of "select" for this hidden
symbol?

Since GENERIC_IRQ_CHIP is a hidden boolean symbol (without a prompt), using
"depends on" makes TC9564_MSIGEN impossible to enable on architectures or
platforms that do not already natively select GENERIC_IRQ_CHIP.

> +	default TC9564_PCI
> +	help
> +	  This enables support for the "MSIGEN" MSI distributor embedded
> +	  in the Toshiba TC9564.  This device receives wired interrupts
> +	  within the TC9564 and raises them to the host using MSI.
> +
>  endmenu
> diff --git a/drivers/irqchip/irq-tc9564-msigen.c b/drivers/irqchip/irq-tc9564-msigen.c
> new file mode 100644
> index 0000000000000..a27edd5859e43
> --- /dev/null
> +++ b/drivers/irqchip/irq-tc9564-msigen.c
[ ... ]
> +static void tc9564_msigen_irq_domain_exit(struct irq_domain *irq_domain)
> +{
> +	struct tc9564_msigen *msigen = irq_domain->host_data;
> +
> +	irq_set_chained_handler_and_data(msigen->irq, NULL, NULL);

[Severity: High]
Does this missing synchronization lead to a use-after-free?

When the chained handler is set to NULL during module removal, it doesn't wait
for currently executing handlers to finish. If tc9564_msigen_irq_handler() is
running concurrently on another CPU, it might access the irq_domain, generic
chip, and unmapped register memory after they are freed by the removal path.

Should this call synchronize_irq() to ensure the concurrent handler has
finished?

> +}
[ ... ]
> +static int tc9564_msigen_probe(struct platform_device *pdev)
> +{
> +	struct irq_domain_chip_generic_info dgc_info;
> +	struct device *dev = &pdev->dev;
> +	struct irq_domain *irq_domain;
> +	struct tc9564_msigen *msigen;
> +	struct irq_domain_info info;

[Severity: High]
Could these uninitialized stack variables cause a crash?

If info is not zero-initialized (e.g., using = {}), fields like info.parent
will contain stack garbage. When passed to devm_irq_domain_instantiate(),
could this result in dereferencing a wild pointer in __irq_domain_instantiate()
if CONFIG_IRQ_DOMAIN_HIERARCHY is enabled?

> +	struct reset_control *reset;
> +	struct pci_dev *pci_dev;
> +	struct clk *clk;
> +	int ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012190245.3988619-1-elder@riscstar.com?part=2