linux-kernel - Re: [PATCH v2 7/8] bus: brcmstb

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: <20170329101357.GB23442@leverpostej>
Date:   Wed, 29 Mar 2017 11:13:58 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Doug Berger <opendmb@...il.com>
Cc:     robh+dt@...nel.org, catalin.marinas@....com, will.deacon@....com,
        computersforpeace@...il.com, gregory.0xf0@...il.com,
        f.fainelli@...il.com, bcm-kernel-feedback-list@...adcom.com,
        wangkefeng.wang@...wei.com, james.morse@....com, mingo@...nel.org,
        sandeepa.s.prabhu@...il.com, shijie.huang@....com,
        linus.walleij@...aro.org, treding@...dia.com, jonathanh@...dia.com,
        olof@...om.net, mirza.krak@...il.com, suzuki.poulose@....com,
        bgolaszewski@...libre.com, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 7/8] bus: brcmstb_gisb: add notifier handling

Hi Doug,

On Tue, Mar 28, 2017 at 02:34:30PM -0700, Doug Berger wrote:
> Check for GISB arbitration errors through a chained notifier
> when a process dies or a kernel panic occurs.  This allows a
> meaningful diagnostic message to occur along with other
> diagnostic information.
> 
> Notably writes to a bad GISB address on an ARM64 architecture
> kernel cause unrecoverable SError aborts and this allows the
> cause of the abort to be seen.
> 
> Signed-off-by: Doug Berger <opendmb@...il.com>

Thanks for changing this.

> ---
>  drivers/bus/brcmstb_gisb.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/bus/brcmstb_gisb.c b/drivers/bus/brcmstb_gisb.c
> index 500b6bb5c739..774729002b8c 100644
> --- a/drivers/bus/brcmstb_gisb.c
> +++ b/drivers/bus/brcmstb_gisb.c
> @@ -24,6 +24,8 @@
>  #include <linux/of.h>
>  #include <linux/bitops.h>
>  #include <linux/pm.h>
> +#include <linux/kernel.h>
> +#include <linux/kdebug.h>
>  
>  #ifdef CONFIG_ARM
>  #include <asm/bug.h>
> @@ -290,6 +292,25 @@ static irqreturn_t brcmstb_gisb_tea_handler(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +/*
> + * Dump out gisb errors on die or panic.
> + */
> +static int dump_gisb_error(struct notifier_block *self, unsigned long v,
> +			   void *p)
> +{
> +	struct brcmstb_gisb_arb_device *gdev;
> +
> +	/* iterate over each GISB arb registered handlers */
> +	list_for_each_entry(gdev, &brcmstb_gisb_arb_device_list, next)
> +		brcmstb_gisb_arb_decode_addr(gdev, "async abort");
> +
> +	return 0;

I think this should be NOTIFY_OK.

> +}
> +
> +static struct notifier_block gisb_error_notifier = {
> +	.notifier_call = dump_gisb_error,
> +};
> +
>  static DEVICE_ATTR(gisb_arb_timeout, S_IWUSR | S_IRUGO,
>  		gisb_arb_get_timeout, gisb_arb_set_timeout);
>  
> @@ -408,6 +429,12 @@ static int __init brcmstb_gisb_arb_probe(struct platform_device *pdev)
>  	board_be_handler = brcmstb_bus_error_handler;
>  #endif
>  
> +	if (list_is_singular(&brcmstb_gisb_arb_device_list)) {
> +		register_die_notifier(&gisb_error_notifier);
> +		atomic_notifier_chain_register(&panic_notifier_list,
> +					       &gisb_error_notifier);

I don't think this is quite right. A notifier_block can only be
registered to one notifier chain at a time, and this has the potential
to corrupt both chains.

I also think you only need to register the panic notifier. An SError
should always result in a panic.

Thanks,
Mark.