linux-kernel - Re: [PATCH] iommu/rockchip: Perform a reset on shutdown

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: <cb32959d-5fc1-c59b-4329-c5c0fa83910b@arm.com>
Date:   Wed, 21 Feb 2018 11:20:53 +0000
From:   Robin Murphy <robin.murphy@....com>
To:     Marc Zyngier <marc.zyngier@....com>,
        Joerg Roedel <joro@...tes.org>,
        Heiko Stuebner <heiko@...ech.de>
Cc:     iommu@...ts.linux-foundation.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Jeffy Chen <jeffy.chen@...k-chips.com>
Subject: Re: [PATCH] iommu/rockchip: Perform a reset on shutdown

Hi Marc,

On 20/02/18 20:25, Marc Zyngier wrote:
> Trying to do a kexec whilst the iommus are still on is proving to be
> a challenging exercise. It is terribly unsafe, as we're reusing the
> memory allocated for the page tables, leading to a likely crash.
> 
> Let's implement a shutdown method that will at least try to stop
> DMA from going crazy behind our back. Note that we need to be
> extra cautious when doing so, as the IOMMU may not be clocked
> if controlled by a another master, as typical on Rockchip system.

Note that clock control is on the way[1], for pretty much that reason; I 
think that series is more or less ready now.

> Suggested-by: Robin Murphy <robin.murphy@....com>
> Signed-off-by: Marc Zyngier <marc.zyngier@....com>
> ---
>   drivers/iommu/rockchip-iommu.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index 9d991c2d8767..6a3719e118da 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -1209,6 +1209,23 @@ static int rk_iommu_remove(struct platform_device *pdev)
>   	return 0;
>   }
>   
> +static void rk_iommu_shutdown(struct platform_device *pdev)
> +{
> +	struct rk_iommu *iommu = platform_get_drvdata(pdev);
> +
> +	/*
> +	 * Be careful not to try to shutdown an otherwise unused
> +	 * IOMMU, as it is likely not to be clocked, and accessing it
> +	 * would just block. An IOMMU without a domain is likely to be
> +	 * unused, so let's use this as a (weak) guard.
> +	 */

Proper clock control will make this comment redundant, but IMO the check 
remains valid either way - rk_iommu::domain being NULL means that no 
devices are currently attached to an active domain, in which case 
translation really should be disabled anyway.

Reviewed-by: Robin Murphy <robin.murphy@....com>

Thanks,
Robin.

[1] 
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1593978.html

> +	if (iommu && iommu->domain) {
> +		rk_iommu_enable_stall(iommu);
> +		rk_iommu_disable_paging(iommu);
> +		rk_iommu_force_reset(iommu);
> +	}
> +}
> +
>   static const struct of_device_id rk_iommu_dt_ids[] = {
>   	{ .compatible = "rockchip,iommu" },
>   	{ /* sentinel */ }
> @@ -1218,6 +1235,7 @@ MODULE_DEVICE_TABLE(of, rk_iommu_dt_ids);
>   static struct platform_driver rk_iommu_driver = {
>   	.probe = rk_iommu_probe,
>   	.remove = rk_iommu_remove,
> +	.shutdown = rk_iommu_shutdown,
>   	.driver = {
>   		   .name = "rk_iommu",
>   		   .of_match_table = rk_iommu_dt_ids,
>