linux-kernel - Re: [RFC PATCH] irqchip/gic-v3: Add quirk for msm8996 secured registers

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]

Date:   Wed, 13 Jun 2018 13:59:40 +0100
From:   Marc Zyngier <marc.zyngier@....com>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        sudeep.holla@....com, tglx@...utronix.de, jason@...edaemon.net
Cc:     linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        rnayak@...eaurora.org, bjorn.andersson@...aro.org,
        sboyd@...nel.org, nicolas.dechesne@...aro.org
Subject: Re: [RFC PATCH] irqchip/gic-v3: Add quirk for msm8996 secured
 registers

On 13/06/18 12:43, Srinivas Kandagatla wrote:
> Access to GICR_WAKER is restricted on msm8996 SoC. Its been more

Restricted by what? Firmware? Hypervisor? (most likely the later).

> than 2 years of wait for this to be fixed in firmware which is

This surely bodes very well in this day and age, where firmware update
are becoming just as important as updating your kernel and your
userspace to fix security problems. I'm impressed.

> not going anywhere. So add a quirk to not write to this register.
> With this quirk MSM8996 can atleast boot out of mainline,

at least

> which can help community to work with boards based on MSM8996.
> 
> Without this patch Qualcomm DB820c board reboots when GICR_WAKER
> is written to.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
> ---
>  drivers/irqchip/irq-gic-v3.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 76ea56d779a1..d1bb2c0cce02 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -47,6 +47,8 @@ struct redist_region {
>  	bool			single_redist;
>  };
>  
> +#define GICV3_FLAGS_WORKAROUND_IW_GICR_WAKER	(1ULL << 0)

Drop the GICV3 prefix. What does IW means here? Please provide an
erratum number for this, and add a description to
Documentation/arm64/silicon-errata.txt.

> +
>  struct gic_chip_data {
>  	struct fwnode_handle	*fwnode;
>  	void __iomem		*dist_base;
> @@ -55,6 +57,7 @@ struct gic_chip_data {
>  	struct irq_domain	*domain;
>  	u64			redist_stride;
>  	u32			nr_redist_regions;
> +	u64			flags;
>  	bool			has_rss;
>  	unsigned int		irq_nr;
>  	struct partition_desc	*ppi_descs[16];
> @@ -139,6 +142,9 @@ static void gic_enable_redist(bool enable)
>  	u32 count = 1000000;	/* 1s! */
>  	u32 val;
>  
> +	if (gic_data.flags & GICV3_FLAGS_WORKAROUND_IW_GICR_WAKER)
> +		return;
> +
>  	rbase = gic_data_rdist_rd_base();
>  
>  	val = readl_relaxed(rbase + GICR_WAKER);
> @@ -1064,6 +1070,31 @@ static const struct irq_domain_ops partition_domain_ops = {
>  	.select = gic_irq_domain_select,
>  };
>  
> +static bool __maybe_unused gicv3_enable_quirk_msm8996(void *data)

All the functions are prefixed with gic, not gicv3. The function name
should reflect the erratum number.

> +{
> +	struct gic_chip_data *d = data;
> +
> +	d->flags |= GICV3_FLAGS_WORKAROUND_IW_GICR_WAKER;
> +
> +	return true;
> +}
> +
> +static const struct gic_quirk gicv3_quirks[] = {
> +	{
> +		.desc	= "GICV3: Qualcomm MSM8996 WAKER IW",

Please the erratum number in the message. It should read something like:

		"GICv3: Qualcomm erratum BIGNUMBERHERE"

> +		.iidr	= 0x00001070,	/* MSM8996 */
> +		.mask	= 0x0000ffff,

Please match the full GICD_IIDR register, not just the implementer and
the revision. Unless you expect all the QC systems to have the same
behaviour?

> +		.init	= gicv3_enable_quirk_msm8996,
> +	},
> +};
> +
> +static void gic_v3_enable_quirks(struct gic_chip_data *gic_data)

gic, not gic_v3.

> +{
> +	u32 iidr = readl_relaxed(gic_data->dist_base + GICD_IIDR);
> +
> +	gic_enable_quirks(iidr, gicv3_quirks, gic_data);
> +}
> +
>  static int __init gic_init_bases(void __iomem *dist_base,
>  				 struct redist_region *rdist_regs,
>  				 u32 nr_redist_regions,
> @@ -1126,6 +1157,7 @@ static int __init gic_init_bases(void __iomem *dist_base,
>  	if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
>  		its_init(handle, &gic_data.rdists, gic_data.domain);
>  
> +	gic_v3_enable_quirks(&gic_data);
>  	gic_smp_init();
>  	gic_dist_init();
>  	gic_cpu_init();
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...