linux-kernel - Re: [PATCH] Fixed: ARM64 GIC ITS could not resume from suspend

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>] [day] [month] [year] [list]

Date:   Tue, 13 Apr 2021 11:43:08 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     414777006@...com
Cc:     tglx@...utronix.de, linux-kernel@...r.kernel.org,
        Mengguang Peng <pengmengguang@...tium.com.cn>
Subject: Re: [PATCH] Fixed: ARM64 GIC ITS could not resume from suspend

On Tue, 13 Apr 2021 07:40:10 +0100,
414777006@...com wrote:
> 
> From: Mengguang Peng <pengmengguang@...tium.com.cn>
> 
> - After ITS suspend, in the ATF(arm-trusted-firmware),
>   gicv3_rdistif_init_restore() just restore GICR_CTLR.Enable_LPIs bit
>   of boot cpu.
> 
> - In its_cpu_init_lpis() of kernel, gic_data_rdist()->lpi_enable
>   will block setting GICR_CTLR_ENABLE_LPIS bit of the other CPUs
>   when ITS resume after suspend.
> 
> Signed-off-by: Mengguang Peng <pengmengguang@...tium.com.cn>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index ed46e60..8167397 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -4777,7 +4777,7 @@ static int its_save_disable(void)
>  static void its_restore_enable(void)
>  {
>  	struct its_node *its;
> -	int ret;
> +	int ret, cpu;
>  
>  	raw_spin_lock(&its_lock);
>  	list_for_each_entry(its, &its_nodes, entry) {
> @@ -4831,6 +4831,22 @@ static void its_restore_enable(void)
>  		    GITS_TYPER_HCC(gic_read_typer(base + GITS_TYPER)))
>  			its_cpu_init_collection(its);
>  	}
> +
> +	/*
> +	 * Enable LPIs: firmware just restore GICR_CTLR.Enable_LPIs
> +	 * of boot cpu, the other CPUs also should be restore.
> +	 */
> +	for_each_online_cpu(cpu) {
> +		void __iomem *rbase = gic_data_rdist_cpu(cpu)->rd_base;
> +		u32 val;
> +
> +		/* Enable LPIs */
> +		val = readl_relaxed(rbase + GICR_CTLR);
> +		if (val)
> +			continue;
> +		val |= GICR_CTLR_ENABLE_LPIS;
> +		writel_relaxed(val, rbase + GICR_CTLR);
> +	}
>  	raw_spin_unlock(&its_lock);
>  }

This looks completely flawed. If GICR_CTLR.Enable_LPIs is clear on
resume, how can you trust the rest of the bits in the register? How
can you trust *any* register in the redistributor?

And what makes you think it is valid or safe to blindly enable LPIs
without even checking whether they were enabled the first place?

I will reiterate my take on this: if the firmware messes with the RD
on suspend, please address the problem in the firmware so that the RDs
are correctly restored on each CPU.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.