linux-kernel - Re: [PATCH 1/2] x86/amd-iommu: enable iommu before attaching devices

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: <20100402155932.GA3516@redhat.com>
Date:	Fri, 2 Apr 2010 11:59:32 -0400
From:	Vivek Goyal <vgoyal@...hat.com>
To:	Chris Wright <chrisw@...s-sol.org>
Cc:	Joerg Roedel <joerg.roedel@....com>,
	Neil Horman <nhorman@...driver.com>,
	Neil Horman <nhorman@...hat.com>, kexec@...ts.infradead.org,
	linux-kernel@...r.kernel.org, hbabu@...ibm.com,
	iommu@...ts.linux-foundation.org,
	"Eric W. Biederman" <ebiederm@...ssion.com>
Subject: Re: [PATCH 1/2] x86/amd-iommu: enable iommu before attaching
	devices

On Thu, Apr 01, 2010 at 06:23:53PM -0700, Chris Wright wrote:
> Hit another kdump problem as reported by Neil Horman.  When initializaing
> the IOMMU, we attach devices to their domains before the IOMMU is
> fully (re)initialized.  Attaching a device will issue some important
> invalidations.  In the context of the newly kexec'd kdump kernel, the
> IOMMU may have stale cached data from the original kernel.  Because we
> do the attach too early, the invalidation commands are placed in the new
> command buffer before the IOMMU is updated w/ that buffer.  This leaves
> the stale entries in the kdump context and can renders device unusable.
> Simply enable the IOMMU before we do the attach.
> 
> Cc: Neil Horman <nhorman@...driver.com>
> Cc: Vivek Goyal <vgoyal@...hat.com>
> Signed-off-by: Chris Wright <chrisw@...s-sol.org>
> ---
>  arch/x86/kernel/amd_iommu_init.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> --- a/arch/x86/kernel/amd_iommu_init.c
> +++ b/arch/x86/kernel/amd_iommu_init.c
> @@ -1288,6 +1288,8 @@ static int __init amd_iommu_init(void)
>  	if (ret)
>  		goto free;
>  
> +	enable_iommus();
> +
>  	if (iommu_pass_through)
>  		ret = amd_iommu_init_passthrough();
>  	else
> @@ -1300,8 +1302,6 @@ static int __init amd_iommu_init(void)
>  
>  	amd_iommu_init_notifier();
>  
> -	enable_iommus();
> -

Ok, so now we do enable_iommu() before we attach devices and flush DTE and
domain PDE, IO TLB. That makes sense.

Following is just for my education purposes. Trying to understand better
the impact of in-flight DMAs. So IIUC, in kudmp context following seems to be
sequence of events.

1. kernel crashes, we leave IOMMU enabled.

2. boot into capture kernel and we call enable_iommus(). This function
   first disables iommu, sets up new device table and enables iommus
   again.
	a. So during this small window when iommu is disabled and we enable
	   it back, any inflight DMA will passthrough possibly to an
	   unintended physical address as translation is disabled and it
	   can corrupt the kdump kenrel.

	b. Even after enabling the iommu, I guess we will continue to
	   use cached DTE, and translation information to handle any
	   in-flight DMA. The difference is that now iommus are enabled
	   so any in-flight DMA should go to the address as intended in
	   first kenrel and should not corrupt anything.

3. Once iommus are enabled again, we allocated and initilize protection
   domains. We attach devices to domains. In the process we flush the
   DTE, PDE and IO TLBs.

	c. Looks like do_attach->set_dte_entry(), by default gives write
	   permission (IW) to all the devices. I am assuming that at
	   this point of time translation is enabled and possibly unity
	   mapped. If that's the case, any in-flight DMA will not be
	   allowed to happen at unity mapped address and this can possibly
	   corrupt the kernel? 

I understand this patch should fix the case when in second kernel a 
device is not doing DMA because of possibly cached DTE, and translation
information. But looks like in-flight DMA issues will still need a closer
look. But that is a separate issue and needs to be addressed in separate
set of patches.

Thanks
Vivek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/