Re: [PATCH v3 4/4] xen/arm: handle irq_set_type() failures

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi,

Mykola Kvach <mykola_kvach@xxxxxxxx> writes:

> Several Arm firmware initialization paths discard irq_set_type()'s return
> value, violating MISRA C Rule 17.7. If trigger configuration fails,
> initialization continues with an IRQ that was not configured as requested.
>
> GTDT and MADT retain rejected timer and maintenance INTIDs.
> check_timer_irq_cfg() and release_irq() later perform unconditional
> descriptor lookups on those values. Xen has no backing descriptors for
> INTIDs 1024 through 4095, so retaining one can cause an out-of-bounds
> access.
>
> Check the return value in the GTDT, MADT, SPCR, and FF-A paths. Store timer
> INTIDs only after successful trigger configuration, make GTDT parsing
> failure fatal, and stop UART or notification setup when trigger
> configuration fails.
>
> Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>

Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>

> ---
> Changes in v3:
> - Avoid partial state updates and simplify maintenance IRQ setup.
>
> Changes in v2:
> - New patch.
> ---
>  xen/arch/arm/gic-v2.c        | 15 +++++++++------
>  xen/arch/arm/gic-v3.c        | 15 +++++++++------
>  xen/arch/arm/tee/ffa_notif.c | 11 ++++++++++-
>  xen/arch/arm/time.c          | 18 ++++++++++++++----
>  xen/drivers/char/ns16550.c   |  8 ++++++--
>  xen/drivers/char/pl011.c     |  4 +++-
>  6 files changed, 51 insertions(+), 20 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 43a379fdda..a24d387e7d 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -1166,17 +1166,20 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header 
> *header,
>      /* Read from APIC table and fill up the GIC variables */
>      if ( cpu_base_assigned == 0 )
>      {
> +        int rc;
> +
> +        rc = irq_set_type(processor->vgic_interrupt,
> +                          processor->flags & ACPI_MADT_VGIC_IRQ_MODE ?
> +                          IRQ_TYPE_EDGE_BOTH : IRQ_TYPE_LEVEL_MASK);
> +
> +        if ( rc )
> +            return rc;
> +
>          cbase = processor->base_address;
>          csize = SZ_8K;
>          hbase = processor->gich_base_address;
>          vbase = processor->gicv_base_address;
>          gicv2_info.maintenance_irq = processor->vgic_interrupt;
> -
> -        if ( processor->flags & ACPI_MADT_VGIC_IRQ_MODE )
> -            irq_set_type(gicv2_info.maintenance_irq, IRQ_TYPE_EDGE_BOTH);
> -        else
> -            irq_set_type(gicv2_info.maintenance_irq, IRQ_TYPE_LEVEL_MASK);
> -
>          cpu_base_assigned = 1;
>      }
>      else
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index acdac22953..b32a9b5009 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1743,15 +1743,18 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header 
> *header,
>      /* Read from APIC table and fill up the GIC variables */
>      if ( !cpu_base_assigned )
>      {
> +        int rc;
> +
> +        rc = irq_set_type(processor->vgic_interrupt,
> +                          processor->flags & ACPI_MADT_VGIC_IRQ_MODE ?
> +                          IRQ_TYPE_EDGE_BOTH : IRQ_TYPE_LEVEL_MASK);
> +
> +        if ( rc )
> +            return rc;
> +
>          cbase = processor->base_address;
>          vbase = processor->gicv_base_address;
>          gicv3_info.maintenance_irq = processor->vgic_interrupt;
> -
> -        if ( processor->flags & ACPI_MADT_VGIC_IRQ_MODE )
> -            irq_set_type(gicv3_info.maintenance_irq, IRQ_TYPE_EDGE_BOTH);
> -        else
> -            irq_set_type(gicv3_info.maintenance_irq, IRQ_TYPE_LEVEL_MASK);
> -
>          cpu_base_assigned = 1;
>      }
>      else
> diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
> index 186e726412..d08d0a3366 100644
> --- a/xen/arch/arm/tee/ffa_notif.c
> +++ b/xen/arch/arm/tee/ffa_notif.c
> @@ -407,7 +407,16 @@ void ffa_notif_init(void)
>          irq = resp.a2;
>          notif_sri_irq = irq;
>          if ( irq >= NR_GIC_SGI )
> -            irq_set_type(irq, IRQ_TYPE_EDGE_RISING);
> +        {
> +            ret = irq_set_type(irq, IRQ_TYPE_EDGE_RISING);
> +            if ( ret )
> +            {
> +                printk(XENLOG_ERR
> +                       "ffa: irq_set_type irq %u failed: error %d\n",
> +                       irq, ret);
> +                return;
> +            }
> +        }
>          ret = request_irq(irq, 0, notif_irq_handler, "FF-A notif", NULL);
>          if ( ret )
>          {
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index 6955b2788f..39b5eabe7c 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -60,20 +60,27 @@ static int __init arch_timer_acpi_init(struct 
> acpi_table_header *header)
>  {
>      u32 irq_type;
>      struct acpi_table_gtdt *gtdt;
> +    int rc;
>  
>      gtdt = container_of(header, struct acpi_table_gtdt, header);
>  
>      /* Initialize all the generic timer IRQ variable from GTDT table */
>      irq_type = acpi_get_timer_irq_type(gtdt->non_secure_el1_flags);
> -    irq_set_type(gtdt->non_secure_el1_interrupt, irq_type);
> +    rc = irq_set_type(gtdt->non_secure_el1_interrupt, irq_type);
> +    if ( rc )
> +        return rc;
>      timer_irq[TIMER_PHYS_NONSECURE_PPI] = gtdt->non_secure_el1_interrupt;
>  
>      irq_type = acpi_get_timer_irq_type(gtdt->virtual_timer_flags);
> -    irq_set_type(gtdt->virtual_timer_interrupt, irq_type);
> +    rc = irq_set_type(gtdt->virtual_timer_interrupt, irq_type);
> +    if ( rc )
> +        return rc;
>      timer_irq[TIMER_VIRT_PPI] = gtdt->virtual_timer_interrupt;
>  
>      irq_type = acpi_get_timer_irq_type(gtdt->non_secure_el2_flags);
> -    irq_set_type(gtdt->non_secure_el2_interrupt, irq_type);
> +    rc = irq_set_type(gtdt->non_secure_el2_interrupt, irq_type);
> +    if ( rc )
> +        return rc;
>      timer_irq[TIMER_HYP_PPI] = gtdt->non_secure_el2_interrupt;
>  
>      return 0;
> @@ -81,7 +88,10 @@ static int __init arch_timer_acpi_init(struct 
> acpi_table_header *header)
>  
>  static void __init preinit_acpi_xen_time(void)
>  {
> -    acpi_table_parse(ACPI_SIG_GTDT, arch_timer_acpi_init);
> +    int rc = acpi_table_parse(ACPI_SIG_GTDT, arch_timer_acpi_init);
> +
> +    if ( rc )
> +        panic("Timer: Failed to configure interrupts from GTDT: %d\n", rc);
>  }
>  #else
>  static void __init preinit_acpi_xen_time(void) { }
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 120ac09d23..eb608ab8b4 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1928,6 +1928,7 @@ static int __init ns16550_acpi_uart_init(const void 
> *data)
>      struct acpi_table_header *table;
>      struct acpi_table_spcr *spcr;
>      acpi_status status;
> +    int rc;
>      /*
>       * Same as the DT part.
>       * Only support one UART on ARM which happen to be ns16550_com[0].
> @@ -1959,6 +1960,11 @@ static int __init ns16550_acpi_uart_init(const void 
> *data)
>          return -EINVAL;
>      }
>  
> +    /* The trigger/polarity information is not available in spcr. */
> +    rc = irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
> +    if ( rc )
> +        return rc;
> +
>      ns16550_init_common(uart);
>  
>      /*
> @@ -1975,8 +1981,6 @@ static int __init ns16550_acpi_uart_init(const void 
> *data)
>      uart->reg_shift = spcr->serial_port.bit_offset;
>      uart->reg_width = spcr->serial_port.access_width;
>  
> -    /* The trigger/polarity information is not available in spcr. */
> -    irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
>      uart->irq = spcr->interrupt;
>  
>      uart->vuart.base_addr = uart->io_base;
> diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
> index a336241033..97c53c11e0 100644
> --- a/xen/drivers/char/pl011.c
> +++ b/xen/drivers/char/pl011.c
> @@ -363,7 +363,9 @@ static int __init pl011_acpi_uart_init(const void *data)
>              spcr->interface_type == ACPI_DBG2_SBSA_32);
>  
>      /* trigger/polarity information is not available in spcr */
> -    irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
> +    res = irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
> +    if ( res )
> +        return res;
>  
>      /* TODO - mmio32 proper handling (for now set to true) */
>      res = pl011_uart_init(spcr->interrupt, spcr->serial_port.address,

-- 
WBR, Volodymyr