linux-kernel - Re: [PATCH v0] irqchip/gic-v3: Avoid check of lpi configuration for non existent cpu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0101016ed626c22f-170ea43c-0db3-40aa-b360-21db453b391b-000000@us-west-2.amazonses.com>
Date:   Thu, 5 Dec 2019 13:01:34 +0000
From:   Gaurav Kohli <gkohli@...eaurora.org>
To:     Marc Zyngier <maz@...nel.org>
Cc:     tglx@...utronix.de, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v0] irqchip/gic-v3: Avoid check of lpi configuration for
 non existent cpu



On 12/5/2019 6:17 PM, Marc Zyngier wrote:
> Hi Gaurav,
> 
> On 2019-12-05 10:55, Gaurav Kohli wrote:
>> As per GIC specification, we can configure gic for more no of cpus
>> then the available cpus in the soc, But this can cause mem abort
>> while iterating lpi region for non existent cpu as we don't map
> 
> Which LPI region? We're talking about RDs, right... Or does LPI mean
> something other than GIC LPIs for you?
> 

Yes RDs only.
>> redistrubutor region for non-existent cpu.
>>
>> To avoid this issue, put one more check of valid mpidr.
> 
> Sorry, but I'm not sure I grasp your problem. Let me try and rephrase it:
> 
> - Your GIC is configured for (let's say) 8 CPUs, and your SoC has only 4.
Yes, suppose gic is configured for 8 cpus but soc has only 4 cpus. Then 
in this case gic_iterate will iterate till it get TYPER_LAST.

But as gic is configured for 8, So last bit sets in eight redistributor 
regions only.
> 
> - As part of the probing, the driver iterates on the RD regions and 
> explodes
>    because something isn't mapped?
> 
> That'd be a grave bug, but I believe the issue is somewhere else.

There are 4 cpus present, that's why we have mapped 4 redistributor 
only, but during probe below function keeps iterating and give mem abort 
for 5th cpu.

static void gic_update_vlpi_properties(void)
{
         gic_iterate_rdists(__gic_update_vlpi_properties);

}

We can solve this problem by mapping all eight redistributor in dt, but 
ideally code should also able to handle this and we can avoid mappin?
>
>>
>> Signed-off-by: Gaurav Kohli <gkohli@...eaurora.org>
>>
>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>> index 1edc993..adc9186 100644
>> --- a/drivers/irqchip/irq-gic-v3.c
>> +++ b/drivers/irqchip/irq-gic-v3.c
>> @@ -766,6 +766,7 @@ static int gic_iterate_rdists(int (*fn)(struct
>> redist_region *, void __iomem *))
>>  {
>>      int ret = -ENODEV;
>>      int i;
>> +    int cpu = 0;
>>
>>      for (i = 0; i < gic_data.nr_redist_regions; i++) {
>>          void __iomem *ptr = gic_data.redist_regions[i].redist_base;
>> @@ -780,6 +781,7 @@ static int gic_iterate_rdists(int (*fn)(struct
>> redist_region *, void __iomem *))
>>          }
>>
>>          do {
>> +            cpu++;
>>              typer = gic_read_typer(ptr + GICR_TYPER);
>>              ret = fn(gic_data.redist_regions + i, ptr);
>>              if (!ret)
>> @@ -795,7 +797,8 @@ static int gic_iterate_rdists(int (*fn)(struct
>> redist_region *, void __iomem *))
>>                  if (typer & GICR_TYPER_VLPIS)
>>                      ptr += SZ_64K * 2; /* Skip VLPI_base + reserved 
>> page */
>>              }
>> -        } while (!(typer & GICR_TYPER_LAST));
>> +        } while (!(typer & GICR_TYPER_LAST) &&
>> +                    cpu_logical_map(cpu) != INVALID_HWID);
>>      }
>>
>>      return ret ? -ENODEV : 0;
> 
> This makes little sense. A redistributor region contains a bunch of RDs,
> each of which maps onto a given CPU. We iterate on the RDs, and not on the
> CPUs, as it is the RD that tells us which CPU it is affine with, not the
> other way around.
> 
> If a RD is for some reason unavailable, then it shouldn't be described in
> the firmware the first place. If you end-up exposing RD regions that do
> not have the last RD having GICR_TYPER.Last set, then your SoC is broken,
> and this needs yet another quirk.
> 
>          M.

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.