linux-kernel - Re: [PATCH] ARM: drivers/amba: release the resource to allow for deferred probe

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

Message-ID: <20191001215042.GO25745@shell.armlinux.org.uk>
Date:   Tue, 1 Oct 2019 22:50:42 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Dinh Nguyen <dinguyen@...nel.org>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linus.walleij@...aro.org, p.zabel@...gutronix.de,
        thor.thayer@...ux.intel.com
Subject: Re: [PATCH] ARM: drivers/amba: release the resource to allow for
 deferred probe

On Tue, Oct 01, 2019 at 04:40:26PM -0500, Dinh Nguyen wrote:
> With commit "79bdcb202a35 ARM: 8906/1: drivers/amba: add reset control to
> amba bus probe", the amba bus driver needs to be deferred probe because the
> reset driver is probed later than the amba bus. However with a deferred
> probe, the call to request_resource() in the driver returns -EBUSY. The
> reason is the driver has not released the resource from the previous probe
> attempt.
> 
> This patch releases the resource when amba_device_try_add() returns
> -EPROBE_DEFER. This allows the deferred probe to continue.
> 
> Fixes: 79bdcb202a35 ("ARM: 8906/1: drivers/amba: add reset control to
> amba bus probe")
> Signed-off-by: Dinh Nguyen <dinguyen@...nel.org>
> ---
>  drivers/amba/bus.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index f39f075abff9..f246b847c991 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -535,6 +535,7 @@ int amba_device_add(struct amba_device *dev, struct resource *parent)
>  
>  	if (ret == -EPROBE_DEFER) {
>  		struct deferred_device *ddev;
> +		release_resource(&dev->res);

This is in the wrong place, and misses more serious leaks.

>  		ddev = kmalloc(sizeof(*ddev), GFP_KERNEL);
>  		if (!ddev)

What we have is bad error cleanup code in amba_device_try_add().
Consider what would happen if dev_pm_domain_attach() inside that
function were to return with -EPROBE_DEFER with your patch in
place - we would call release_resource() twice on the same
resource.  Clearly, that's incorrect.

The problem is that an error from
of_reset_control_array_get_optional_shared() just returns, leaving
everything that amba_device_try_add() already did still in place.
So, for example, a subsequent call to amba_device_try_add() will
remap the resource, leaking the previous mapping.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up