Re: [PATCH] irqchip: gicv2m: Fix use after free of gicv2m_get_fwnode

[Marc Zyngier <maz@kernel.org>]

On Tue, 22 Apr 2025 17:16:16 +0100,
Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> 
> With ACPI in place, we register gicv2m_get_fwnode with the pci subsystem as
> pci_msi_get_fwnode_cb, which may get invoked at runtime during a PCI host bridge
> probe. But, the call back is wrongly marked as __init, causing it to be freed,
> while being registered with the PCI subsystem and could trigger:
> 
>  Unable to handle kernel paging request at virtual address ffff8000816c0400
>  Mem abort info:
>    ESR = 0x0000000086000007
>    EC = 0x21: IABT (current EL), IL = 32 bits
>    SET = 0, FnV = 0
>    EA = 0, S1PTW = 0
>    FSC = 0x07: level 3 translation fault
>  swapper pgtable: 4k pages, 48-bit VAs, pgdp=00000000f2c7d000
>  pgd=10000000f3a0a003, p4d=10000000f3a0a003, pud=10000000f3a0b003, pmd=10000000f3a0e003, pte=0000000000000000
>  PREEMPT SMP
>  Modules linked in: devsec_bus(+) devsec_common
>  CPU: 5 UID: 0 PID: 387 Comm: modprobe Not tainted 6.14.0-rc6+ #1686
>  Hardware name: ARM LTD ARM Juno Development Platform/ARM Juno Development Platform, BIOS EDK II Feb  1 2019
>  pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>  pc : gicv2m_get_fwnode+0x0/0x58
>  lr : pci_host_bridge_acpi_msi_domain+0x38/0xa0

Trim...

>  sp : ffff800083453640
>  x29: ffff800083453640 x28: 0000000000000000 x27: ffff00080443b010
>  x26: ffff0008044b0000 x25: fffffff000000000 x24: ffff00080443b720
>  x23: ffff00080443b800 x22: ffff00080443b010 x21: ffff800083453748
>  x20: ffff000806d56000 x19: ffff000806d56000 x18: 0000000000000030
>  x17: 6f6674616c703d4d x16: 4554535953425553 x15: 00000000ffffffff
>  x14: ffff00080110009c x13: 0000000000000002 x12: ffff00080110009a
>  x11: 0000000000000040 x10: 0000000000000000 x9 : ffff800080792de4
>  x8 : 0101010101010101 x7 : 7f7f7f7f7f7f7f7f x6 : 5e646c68736d7471
>  x5 : ffff00080443b438 x4 : ffff00080443b438 x3 : 0000000000000000
>  x2 : ffff8000816c0400 x1 : ffff0008011a3480 x0 : ffff000806d56120

... this (not specially useful).

>  Call trace:
>   gicv2m_get_fwnode+0x0/0x58 (P)
>   pci_set_bus_msi_domain+0x74/0x88
>   pci_register_host_bridge+0x194/0x548
>   devsec_bus_probe+0x410/0xf28 [devsec_bus]
>   platform_probe+0x70/0xe8
>   really_probe+0xc4/0x2b0
>   __driver_probe_device+0x80/0x140
>   driver_probe_device+0xe4/0x170
>   __driver_attach+0x9c/0x1b0
>   bus_for_each_dev+0x7c/0xe8
>   driver_attach+0x2c/0x40
>   bus_add_driver+0xec/0x218
>   driver_register+0x68/0x138
>   __platform_driver_probe+0x5c/0xe8
>   devsec_bus_init+0x88/0xd0 [devsec_bus]
>   do_one_initcall+0x4c/0x280
>   do_init_module+0x60/0x230
>   load_module+0x1ce8/0x1dd8
>   init_module_from_file+0x90/0xd8
> 
> Retain the function for later use. Easily recreated with [1], on Juno board with
> ACPI boot.
> 
> [1] https://lkml.kernel.org/r/68053cf43bb54_7205294cc@dwillia2-xfh.jf.intel.com.notmuch
> 
> Fixes: 0644b3daca28 ("irqchip/gic-v2m: acpi: Introducing GICv2m ACPI support")
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
> I am not sure if this needs to goto stable, nobody seems to have hit this ? But hey
> it doesn't hurt.

I stopped following what goes to stable and what doesn't. Irrespective
of what you add to the commit message, the stable maintainers do their
own thing.

> ---
>  drivers/irqchip/irq-gic-v2m.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c
> index be35c5349986..a1e370d0200f 100644
> --- a/drivers/irqchip/irq-gic-v2m.c
> +++ b/drivers/irqchip/irq-gic-v2m.c
> @@ -423,7 +423,7 @@ static int __init gicv2m_of_init(struct fwnode_handle *parent_handle,
>  #ifdef CONFIG_ACPI
>  static int acpi_num_msi;
>  
> -static __init struct fwnode_handle *gicv2m_get_fwnode(struct device *dev)
> +static struct fwnode_handle *gicv2m_get_fwnode(struct device *dev)
>  {
>  	struct v2m_data *data;
>  

Otherwise, nice catch.

Reviewed-by: Marc Zyngier <maz@kernel.org>

	M.

-- 
Without deviation from the norm, progress is not possible.