Re: [3/3] irqchip/gic-v3: Bounds check redistributor accesses

From: Lokesh Vutla
Date: Tue Mar 13 2018 - 09:40:03 EST


Hi All,

On Wednesday 11 October 2017 03:11 PM, Punit Agrawal wrote:
> The kernel crashes while iterating over a redistributor that is
> in-correctly sized by the platform firmware or doesn't contain the last
> record.
>
> Prevent the crash by checking accesses against the size of the region
> provided by the firmware. While we are at it, warn the user about
> incorrect region size.
>
> Signed-off-by: Punit Agrawal <punit.agrawal@xxxxxxx>
> Cc: Marc Zyngier <marc.zyngier@xxxxxxx>

Sorry to bring up an old thread. Just wanted to check what is the status
on this series.

This will also be useful when we try to boot linux + hypervisor with
less number of cores than the SoC supports. For example:
- SoC has 4 cores and Linux tries to boot with 2 cores.
- then a type-2 hypervisor gets installed.
- Hypervisor tries to boot a VM with linux on core 1.

Now the VM boot will fail while it iterates over all the GICR regions
till GICR_TYPER is found. Hypervisor will trap any accesses to GICR
regions of any invalid cpus(cpu 2, cpu 3 in this case).

If the $patch is not the right approach, can you suggest on how to
handle the above scenario?

Thanks and regards,
Lokesh

> ---
> drivers/irqchip/irq-gic-v3.c | 48 ++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 40 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 881d327c53fa..754d936c95e5 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -429,11 +429,21 @@ static int gic_iterate_rdists(int (*fn)(struct redist_region *, void __iomem *))
> int i;
>
> for (i = 0; i < gic_data.nr_redist_regions; i++) {
> - void __iomem *ptr = gic_data.redist_regions[i].redist_base;
> struct resource *res = &gic_data.redist_regions[i].res;
> - u64 typer;
> + void __iomem *ptr, *base;
> + u64 typer, size, stride;
> u32 reg;
>
> + ptr = base = gic_data.redist_regions[i].redist_base;
> + size = resource_size(res);
> +
> + stride = gic_data.redist_stride ?: SZ_64K * 2;
> + if (ptr + stride > base + size) {
> + pr_warn("Insufficient size for redistributor region @%llx. Skipping\n",
> + res->start);
> + continue;
> + }
> +
> reg = readl_relaxed(ptr + GICR_PIDR2) & GIC_PIDR2_ARCH_MASK;
> if (reg != GIC_PIDR2_ARCH_GICv3 &&
> reg != GIC_PIDR2_ARCH_GICv4) { /* We're in trouble... */
> @@ -442,7 +452,28 @@ static int gic_iterate_rdists(int (*fn)(struct redist_region *, void __iomem *))
> }
>
> do {
> + /*
> + * We can access GICR_TYPER as we have already
> + * checked that we have atleast 128kB or
> + * redist_stride
> + */
> typer = gic_read_typer(ptr + GICR_TYPER);
> + if (!gic_data.redist_stride &&
> + (typer & GICR_TYPER_VLPIS)) {
> + /* VLPI_base + reserved page */
> + stride += SZ_64K * 2;
> +
> + /*
> + * We are larger than we thought, do
> + * we still fit?
> + */
> + if (ptr + stride > base + size) {
> + pr_warn("No last record found in redistributor region @%llx\n",
> + gic_data.redist_regions[i].res.start);
> + break;
> + }
> + }
> +
> ret = fn(gic_data.redist_regions + i, ptr);
> if (!ret)
> return 0;
> @@ -450,12 +481,13 @@ static int gic_iterate_rdists(int (*fn)(struct redist_region *, void __iomem *))
> if (gic_data.redist_regions[i].single_redist)
> break;
>
> - if (gic_data.redist_stride) {
> - ptr += gic_data.redist_stride;
> - } else {
> - ptr += SZ_64K * 2; /* Skip RD_base + SGI_base */
> - if (typer & GICR_TYPER_VLPIS)
> - ptr += SZ_64K * 2; /* Skip VLPI_base + reserved page */
> + ptr += stride;
> +
> + stride = gic_data.redist_stride ?: SZ_64K * 2;
> + if (ptr + stride > base + size) {
> + pr_warn("No last record found in redistributor region @%llx\n",
> + gic_data.redist_regions[i].res.start);
> + break;
> }
> } while (!(typer & GICR_TYPER_LAST));
> }
>