linux-kernel - Re: [PATCH] irqchip/gic-v3: do not access GICR

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: <86d0ww7ymc.wl-marc.zyngier@arm.com>
Date:   Tue, 12 Jun 2018 17:34:03 +0100
From:   Marc Zyngier <marc.zyngier@....com>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc:     tglx@...utronix.de, jason@...edaemon.net,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        rnayak@...eaurora.org, bjorn.andersson@...aro.org, sboyd@...nel.org
Subject: Re: [PATCH] irqchip/gic-v3: do not access GICR_WAKER if its secured register.

On Tue, 12 Jun 2018 15:55:16 +0100,
Srinivas Kandagatla <srinivas.kandagatla@...aro.org> wrote:
> 
> GICR_WAKER can be a secured register, check this before accessing it
> as its done in power management code.

NAK.

>From the GICv3 spec:

* When GICD_CTLR.DS==1, this register is always accessible.
* When GICD_CTLR.DS==0, this is a Secure register. This register is
  RAZ/WI to Non-secure accesses.

> Without this patch Qualcomm DB820c board crashes.

I suggest you find out how the GIC has been integrated on this
platform. If you take a fault on accessing this register, this very
much looks like an integration bug, and it should be quirked as such.

Thanks,

	M.
	
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
> ---
>  drivers/irqchip/irq-gic-v3.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 5a67ec084588..38136d6e9ca5 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -656,6 +656,12 @@ static int gic_dist_supports_lpis(void)
>  	return !!(readl_relaxed(gic_data.dist_base + GICD_TYPER) & GICD_TYPER_LPIS) && !gicv3_nolpi;
>  }
>  
> +/* Check whether it's single security state view */
> +static bool gic_dist_security_disabled(void)
> +{
> +	return readl_relaxed(gic_data.dist_base + GICD_CTLR) & GICD_CTLR_DS;
> +}
> +
>  static void gic_cpu_init(void)
>  {
>  	void __iomem *rbase;
> @@ -664,7 +670,8 @@ static void gic_cpu_init(void)
>  	if (gic_populate_rdist())
>  		return;
>  
> -	gic_enable_redist(true);
> +	if (gic_dist_security_disabled())
> +		gic_enable_redist(true);
>  
>  	rbase = gic_data_rdist_sgi_base();
>  
> @@ -819,11 +826,6 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
>  #endif
>  
>  #ifdef CONFIG_CPU_PM
> -/* Check whether it's single security state view */
> -static bool gic_dist_security_disabled(void)
> -{
> -	return readl_relaxed(gic_data.dist_base + GICD_CTLR) & GICD_CTLR_DS;
> -}
>  
>  static int gic_cpu_pm_notifier(struct notifier_block *self,
>  			       unsigned long cmd, void *v)
> -- 
> 2.16.2
> 

-- 
Jazz is not dead, it just smell funny.