Re: [PATCH v5 02/49] irqchip/gic-v5: Provide OF IRS config frame attrs to KVM

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] `gic_of_setup_kvm_info()` unconditionally dereferences `irs_data`, which may be NULL if CPU 0 is not connected to a GICv5 IRS.

Pre-existing issues:
- [High] Failure during IRS initialization leaves dangling pointers in `per_cpu_irs_data`, leading to a Use-After-Free.
--

--- Patch [2]: [PATCH v5 02/49] irqchip/gic-v5: Provide OF IRS config frame attrs to KVM ---
commit 1b935b47d4b6186ec42dafa24166af9a1d8adb3d
Author: Sascha Bischoff <[email protected]>
Subject: irqchip/gic-v5: Provide OF IRS config frame attrs to KVM

This commit passes a pointer to an IRS config frame to KVM via the
gic_kvm_info structure. It uses the IRS config frame from CPU 0 as
a representative frame, assuming all IRS config frames are consistent,
and passes along coherency flags.

> diff --git a/drivers/irqchip/irq-gic-v5-irs.c b/drivers/irqchip/irq-gic-v5-irs.c
> index f3fce0b1e25d9..607e066821b52 100644
> --- a/drivers/irqchip/irq-gic-v5-irs.c
> +++ b/drivers/irqchip/irq-gic-v5-irs.c
[ ... ]
> @@ -50,6 +48,11 @@ static void irs_writeq_relaxed(struct gicv5_irs_chip_data *irs_data,
>  	writeq_relaxed(val, irs_data->irs_base + reg_offset);
>  }
>  
> +struct gicv5_irs_chip_data *gicv5_irs_get_chip_data(void)
> +{
> +	return per_cpu(per_cpu_irs_data, 0);
> +}
> +

[Severity: High]
This is a pre-existing issue, but if gicv5_irs_init() fails during IRS
initialization, a dangling pointer may be left in per_cpu_irs_data:

drivers/irqchip/irq-gic-v5-irs.c:gicv5_irs_of_init() {
    ...
	ret = gicv5_irs_of_init_affinity(node, irs_data, iaffid_bits);
    ...
	ret = gicv5_irs_init(irs_data);
	if (ret)
		goto out_iomem;
    ...
out_err:
	kfree(irs_data);
	return ret;
}

The error path frees irs_data without clearing the pointers previously populated
in per_cpu_irs_data by gicv5_irs_of_init_affinity() (and the ACPI equivalent).
Since IRS probing ignores failures of individual IRS nodes, a dangling pointer
remains in the per-CPU array.

Will returning that dangling pointer here lead to a Use-After-Free when it is
later accessed?

> diff --git a/drivers/irqchip/irq-gic-v5.c b/drivers/irqchip/irq-gic-v5.c
> index c04523a438cee..e7a7aedcfaf78 100644
> --- a/drivers/irqchip/irq-gic-v5.c
> +++ b/drivers/irqchip/irq-gic-v5.c
> @@ -1128,6 +1128,8 @@ static struct gic_kvm_info gic_v5_kvm_info __initdata;
>  
>  static void __init gic_of_setup_kvm_info(struct device_node *node)
>  {
> +	struct gicv5_irs_chip_data *irs_data = gicv5_irs_get_chip_data();
> +
[ ... ]
> @@ -1140,6 +1142,9 @@ static void __init gic_of_setup_kvm_info(struct device_node *node)
>  
>  	gic_v5_kvm_info.type = GIC_V5;
>  
> +	gic_v5_kvm_info.gicv5_irs.base = irs_data->irs_base;
> +	gic_v5_kvm_info.gicv5_irs.non_coherent = !!(irs_data->flags & IRS_FLAGS_NON_COHERENT);
> +

[Severity: High]
If CPU 0 is not explicitly listed in the 'cpus' device tree property for any
IRS (meaning CPU 0 lacks an IRS), gicv5_irs_get_chip_data() will return NULL.

Can this unconditionally dereference irs_data without a NULL check, potentially
causing a panic during boot on valid topologies?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2