linux-kernel - Re: [PATCH] Enable GART-IOMMU only after setting up protection methods

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: <20090409122720.GB12440@8bytes.org>
Date:	Thu, 9 Apr 2009 14:27:20 +0200
From:	Joerg Roedel <joro@...tes.org>
To:	Mark Langsdorf <mark.langsdorf@....com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...e.hu>
Subject: Re: [PATCH] Enable GART-IOMMU only after setting up protection
	methods

On Wed, Apr 08, 2009 at 04:08:55PM -0500, Mark Langsdorf wrote:
> The current code to set up the GART as an IOMMU enables GART
> translations before it removes the aperture from the kernel
> memory map, sets the GART PTEs to UC, sets up the guard and
> scratch pages, or does a wbinvd().  This leaves the possibility
> of cache aliasing open and can cause system crashes.

This patch looks broken.  I don't think that there are any aliasing
problems in the existing code.  Can you elaborate were do you see these
problems? Is there any existing bug report for this?

> Re-order the code and add tlbflush so as to enable the 
> GART translations only after all safeguards are in place and
> the tlb has been flushed.
> 
> AMD has tested this patch and seen no adverse effects.
> 
> Signed-off-by: Mark Langsdorf <mark.langsdorf@....com>
> 
> diff -r 0d1744c7acc7 arch/x86/kernel/pci-gart_64.c
> --- a/arch/x86/kernel/pci-gart_64.c	Fri Mar 27 16:47:28 2009 -0500
> +++ b/arch/x86/kernel/pci-gart_64.c	Mon Mar 30 15:05:47 2009 -0500
> @@ -38,6 +38,7 @@
>  #include <asm/swiotlb.h>
>  #include <asm/dma.h>
>  #include <asm/k8.h>
> +#include <asm/tlbflush.h>
>  
>  static unsigned long iommu_bus_base;	/* GART remapping area (physical) */
>  static unsigned long iommu_size;	/* size of remapping area bytes */
> @@ -682,9 +683,9 @@
>  	if (set_memory_uc((unsigned long)gatt, gatt_size >> PAGE_SHIFT))
>  		panic("Could not set GART PTEs to uncacheable pages");
>  
> +	wbinvd();
> +
>  	agp_gatt_table = gatt;
> -
> -	enable_gart_translations();

To enable the GART here has the advantage that all other CPUs are not
yet up. Since the GATT has only non-present entries there is no cache
aliasing problem here. If you enable it later you need to flush the
cache on _all_ CPUs and not just the CPU where the GART initialization
code runs.

>  
>  	error = sysdev_class_register(&gart_sysdev_class);
>  	if (!error)
> @@ -855,6 +856,11 @@
>  	for (i = EMERGENCY_PAGES; i < iommu_pages; i++)
>  		iommu_gatt_base[i] = gart_unmapped_entry;
>  
> +	wbinvd();

Why a wbinvd() here? A few lines above is already one.

> +	flush_tlb_all();

Same with TLB flush. The set_memory_np() function does that for us.

> +
> +	enable_gart_translations();
> +

If we change the enablement logic like done in this patch you need to
invalidate the caches on _all_ cpus. I don't think this is necessary if
we just enable all GARTs when only the boot cpu is up.

>  	flush_gart();
>  	dma_ops = &gart_dma_ops;
>  }
--
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/