linux-kernel - Re: [PATCH] irqchip/gic: check return value of of_address_to

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: <20180705201327.4a4dc7dd@why.wild-wind.fr.eu.org>
Date:   Thu, 5 Jul 2018 20:13:27 +0100
From:   Marc Zyngier <marc.zyngier@....com>
To:     Bo Yan <byan@...dia.com>
Cc:     <tglx@...utronix.de>, <jason@...edaemon.net>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] irqchip/gic: check return value of
 of_address_to_resource

Hi Bo,

On Thu, 5 Jul 2018 11:20:59 -0700
Bo Yan <byan@...dia.com> wrote:

> The of_address_to_resource returns 0 if successful. gic_check_eoimode
> calls it without checking the return value. This induces Coverity
> warning: "Unchecked return value".
> 
> Return false from gic_check_eoimode if of_address_to_resource returns
> non-0 value.
> 
> Signed-off-by: Bo Yan <byan@...dia.com>
> ---
>  drivers/irqchip/irq-gic.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index ced10c4..0bceb10 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -1284,7 +1284,8 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
>  {
>  	struct resource cpuif_res;
>  
> -	of_address_to_resource(node, 1, &cpuif_res);
> +	if (of_address_to_resource(node, 1, &cpuif_res))
> +		return false;

We've just done an of_iomap() on this resource, which succeeded. How
can the same thing now fail? It would mean that the device tree has
been pulled from under our feet...

And if it could happen, why is returning false the right thing to do?
Why would we say we want EOImode==0 instead of 1?

>  
>  	if (!is_hyp_mode_available())
>  		return false;

As it stands, I'm not taking such a patch. It either papers over a
bigger problem, or just keeps a warning quiet for the sake of it.

Thanks,

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