Re:Re: [PATCH 3/6] irqchip/gic-v3: validate MADT records before use

[jiale yao]

Hi Marc,
At 2026-10-16 22:36:50, "Marc Zyngier" <maz@xxxxxxxxxx> wrote:
>On Fri, 16 Oct 2026 15:13:54 +0100,
>Jiale Yao <yaojiale02@xxxxxxx> wrote:
>> 
>> The GICv3 ACPI paths cast generic MADT headers to distributor,
>> redistributor, and CPU interface records before confirming that the
>> corresponding fixed-size structures are present. Truncated records can
>> therefore cause out-of-bounds field reads while probing or setting up
>> virtualization data.
>>
>> Check each typed MADT record before its first field access. Also reject
>> a short distributor record in the version validator, which runs before
>> the probe callback.
>> 
>> Signed-off-by: Jiale Yao <yaojiale02@xxxxxxx>
>> ---
>>  drivers/irqchip/irq-gic-v3.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>> 
>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>> index 6e1fa5b247fc..0588d6d83b31 100644
>> --- a/drivers/irqchip/irq-gic-v3.c
>> +++ b/drivers/irqchip/irq-gic-v3.c
>> @@ -2299,6 +2299,9 @@ gic_acpi_parse_madt_redist(union acpi_subtable_headers *header,
>>  			(struct acpi_madt_generic_redistributor *)header;
>>  	void __iomem *redist_base;
>>  
>> +	if (BAD_MADT_ENTRY(redist, end))
>> +		return -EINVAL;
>> +
>>  	redist_base = ioremap(redist->base_address, redist->length);
>>  	if (!redist_base) {
>>  		pr_err("Couldn't map GICR region @%llx\n", redist->base_address);
>> @@ -2325,6 +2328,9 @@ gic_acpi_parse_madt_gicc(union acpi_subtable_headers *header,
>>  	u32 size = reg == GIC_PIDR2_ARCH_GICv4 ? SZ_64K * 4 : SZ_64K * 2;
>>  	void __iomem *redist_base;
>>  
>> +	if (BAD_MADT_ENTRY(gicc, end))
>> +		return -EINVAL;
>> +
>>  	/* Neither enabled or online capable means it doesn't exist, skip it */
>>  	if (!(gicc->flags & (ACPI_MADT_ENABLED | ACPI_MADT_GICC_ONLINE_CAPABLE)))
>>  		return 0;
>> @@ -2391,6 +2397,9 @@ static int __init gic_acpi_match_gicc(union acpi_subtable_headers *header,
>>  	struct acpi_madt_generic_interrupt *gicc =
>>  				(struct acpi_madt_generic_interrupt *)header;
>>  
>> +	if (BAD_MADT_ENTRY(gicc, end))
>> +		return -EINVAL;
>> +
>>  	/*
>>  	 * If GICC is enabled and has valid gicr base address, then it means
>>  	 * GICR base is presented via GICC. The redistributor is only known to
>> @@ -2437,6 +2446,9 @@ static bool __init acpi_validate_gic_table(struct acpi_subtable_header *header,
>>  	int count;
>>  
>>  	dist = (struct acpi_madt_generic_distributor *)header;
>> +	if (header->length < sizeof(*dist))
>> +		return false;
>> +
>
>Why is this open-coded instead of using the checking macro?

Agreed. I will use BAD_MADT_ENTRY() with the end of the current record instead of open-coding the
minimum-size check.

>
>>  	if (dist->version != ape->driver_data)
>>  		return false;
>>  
>> @@ -2457,6 +2469,9 @@ static int __init gic_acpi_parse_virt_madt_gicc(union acpi_subtable_headers *hea
>>  	int maint_irq_mode;
>>  	static int first_madt = true;
>>  
>> +	if (BAD_MADT_ENTRY(gicc, end))
>> +		return -EINVAL;
>> +
>
>Why isn't this using BAD_MADT_GICC_ENTRY()?

Good point. GICC records have a version-dependent size, and requiring sizeof(*gicc) can reject valid records
from older ACPI revisions. I will use BAD_MADT_GICC_ENTRY() here and in the other two GICC callbacks added by
this patch.

>
>>  	if (!(gicc->flags &
>>  	      (ACPI_MADT_ENABLED | ACPI_MADT_GICC_ONLINE_CAPABLE)))
>>  		return 0;
>> @@ -2547,6 +2562,9 @@ gic_acpi_init(union acpi_subtable_headers *header, const unsigned long end)
>>  
>>  	/* Get distributor base address */
>>  	dist = (struct acpi_madt_generic_distributor *)header;
>> +	if (BAD_MADT_ENTRY(dist, end))
>> +		return -EINVAL;
>> +
>
>Why does this need to be re-checked if the validation phased has succeeded?

It does not. The probe callback is invoked with the same record immediately after acpi_validate_gic_table()
succeeds, so the check in gic_acpi_init() is redundant. I will remove it and keep the distributor-size
validation in the validation callback.

>
>>  	acpi_data.dist_base = ioremap(dist->base_address,
>>  				      ACPI_GICV3_DIST_MEM_SIZE);
>>  	if (!acpi_data.dist_base) {
>
>Thanks,
>
>	M.
>
>-- 
>Without deviation from the norm, progress is not possible.

I will fix them in v2, thanks for your review.