linux-kernel - Re: [PATCH] x86/mm/pat: Fix BUG_ON in mmap

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]

Date:	Tue, 5 Apr 2016 13:09:47 +0200
From:	Borislav Petkov <bp@...e.de>
To:	Toshi Kani <toshi.kani@....com>
Cc:	mingo@...nel.org, hpa@...or.com, tglx@...utronix.de,
	ying.huang@...ux.intel.com, x86@...nel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86/mm/pat: Fix BUG_ON in mmap_mem on QEMU/i386

On Fri, Apr 01, 2016 at 04:19:45PM -0600, Toshi Kani wrote:
> The following BUG_ON error was reported on QEMU/i386:
> 
>   kernel BUG at arch/x86/mm/physaddr.c:79!
>   Call Trace:
>   phys_mem_access_prot_allowed
>   mmap_mem
>   ? mmap_region
>   mmap_region
>   do_mmap
>   vm_mmap_pgoff
>   SyS_mmap_pgoff
>   do_int80_syscall_32
>   entry_INT80_32
> 
> after commit edfe63ec97ed ("x86/mtrr: Fix Xorg crashes in Qemu
> sessions").
> 
> PAT is now set to disabled state when MTRRs are disabled...

"... thus reactivating the __pa(high_memory) check in
phys_mem_access_prot_allowed()."

> When the system does not have much memory, 'high_memory' points to

What does "much memory" mean, exactly?

> the maximum memory address + 1, which is empty.  When
> CONFIG_DEBUG_VIRTUAL is also set, __pa() calls __phys_addr(), which
> in turn calls slow_virt_to_phys() for high_memory.  Because
> high_memory does not point to a valid memory address, this address
> is not mapped...

"... and slow_virt_to_phys() returns 0."

> Hence, BUG_ON.
> 
> Use __pa_nodebug() as the code does not expect a valid virtual
> mapping for high_memory.
> 
> Reported-by: kernel test robot <ying.huang@...ux.intel.com>
> Link: https://lkml.org/lkml/2016/4/1/608
> Signed-off-by: Toshi Kani <toshi.kani@....com>
> Thomas Gleixner <tglx@...utronix.de>
> Ingo Molnar <mingo@...nel.org>
> H. Peter Anvin <hpa@...or.com>
> Borislav Petkov <bp@...e.de>
> ---
> This patch is based on -tip.
> ---
>  arch/x86/mm/pat.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
> index c4c3ddc..26b7202 100644
> --- a/arch/x86/mm/pat.c
> +++ b/arch/x86/mm/pat.c
> @@ -792,7 +792,7 @@ int phys_mem_access_prot_allowed(struct file *file, unsigned long pfn,
>  	      boot_cpu_has(X86_FEATURE_K6_MTRR) ||
>  	      boot_cpu_has(X86_FEATURE_CYRIX_ARR) ||
>  	      boot_cpu_has(X86_FEATURE_CENTAUR_MCR)) &&
> -	    (pfn << PAGE_SHIFT) >= __pa(high_memory)) {
> +	    (pfn << PAGE_SHIFT) >= __pa_nodebug(high_memory)) {
>  		pcm = _PAGE_CACHE_MODE_UC;
>  	}
>  #endif

Modulo the minor formulations issues above,

Reviewed-by: Borislav Petkov <bp@...e.de>

AFAIU, it makes sense to do the "nodebug" check here anyway - we
basically only want to *check* the address and if outside of available
memory, map UC. We shouldn't be exploding just because we're checking.

But this is just me, someone should doublecheck this train of thought
for sanity.

Thanks.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--