linux-kernel - Re: [PATCH] irqchip/gic: Don't write to GICD

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: <a6938eb1-b861-b3c7-e1f6-b9ffb57988a4@nvidia.com>
Date:   Fri, 7 Apr 2017 09:49:49 +0300
From:   Mikko Perttunen <mperttunen@...dia.com>
To:     Marc Zyngier <marc.zyngier@....com>, <tglx@...utronix.de>,
        <jason@...edaemon.net>
CC:     <linux-kernel@...r.kernel.org>, <talho@...dia.com>,
        <aniruddhab@...dia.com>, Matt Craighead <mcraighead@...dia.com>
Subject: Re: [PATCH] irqchip/gic: Don't write to GICD_ICFGR0

On 06.04.2017 12:26, Marc Zyngier wrote:
> On 06/04/17 09:17, Mikko Perttunen wrote:
>> From: Matt Craighead <mcraighead@...dia.com>
>>
>> According to the GICv2 specification, the GICD_ICFGR0,
>> or GIC_DIST_CONFIG[0] register is read-only. Therefore
>> avoid writing to it.
>
> Have you verified that this also applies to pre-v2 GICs?

I had not, but I just looked up the GICv1 specification and this also 
applies to GICv1.

>
>>
>> Signed-off-by: Matt Craighead <mcraighead@...dia.com>
>> [mperttunen@...dia.com: commit message rewritten]
>> Signed-off-by: Mikko Perttunen <mperttunen@...dia.com>
>> ---
>>  drivers/irqchip/irq-gic.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>> index 1b1df4f770bd..d9c0000050e0 100644
>> --- a/drivers/irqchip/irq-gic.c
>> +++ b/drivers/irqchip/irq-gic.c
>> @@ -609,7 +609,7 @@ void gic_dist_restore(struct gic_chip_data *gic)
>>
>>  	writel_relaxed(GICD_DISABLE, dist_base + GIC_DIST_CTRL);
>>
>> -	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 16); i++)
>> +	for (i = 1; i < DIV_ROUND_UP(gic_irqs, 16); i++)
>>  		writel_relaxed(gic->saved_spi_conf[i],
>>  			dist_base + GIC_DIST_CONFIG + i * 4);
>>
>> @@ -699,7 +699,7 @@ void gic_cpu_restore(struct gic_chip_data *gic)
>>  	}
>>
>>  	ptr = raw_cpu_ptr(gic->saved_ppi_conf);
>> -	for (i = 0; i < DIV_ROUND_UP(32, 16); i++)
>> +	for (i = 1; i < DIV_ROUND_UP(32, 16); i++)
>>  		writel_relaxed(ptr[i], dist_base + GIC_DIST_CONFIG + i * 4);
>
> Assuming that the above stands for all GICs, it feels like there is room
> for simplification here. But you haven't dealt with the save side, so
> what's the point?
>

Yes, with this we could also drop saving the value when saving, and 
that's probably worth doing. We could also just shift the indexing to be 
one higher always.

> Also, you're missing out some other stuff which is (by definition) RO as
> well, such as the target registers for SGIs and PPIs. Finally, there is
> the question of the allocated memory for these registers.

At least for the target register, the driver already seems to have code 
to skip the fields defined as read-only. I havent looked for other 
read-only registers, but this is the only registers we are having issues 
with (see below).

>
> Overall, I'm not sure what this patch is trying to achieve. It doesn't
> fix a bug, and is not complete enough to do something useful (even
> though it would only be saving a handful of bytes).
>
> Maybe you can explain what you're trying to do here?

Sure. Our simulation environment enforces the read-only-ness of these 
registers, so the driver as is doesn't work in simulation. As far as I 
understand, the register being read-only means that the model is allowed 
to do this.

>
> Thanks,
>
> 	M.
>

Thanks for reviewing!

Mikko