linux-kernel - Re: [PATCH 00 of 10] x86: unify asm/pgtable.h

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, 08 Jan 2008 16:53:29 -0800
From:	Jeremy Fitzhardinge <jeremy@...p.org>
To:	Ingo Molnar <mingo@...e.hu>
CC:	LKML <linux-kernel@...r.kernel.org>, Andi Kleen <ak@...e.de>,
	Glauber de Oliveira Costa <glommer@...il.com>,
	Jan Beulich <jbeulich@...ell.com>
Subject: Re: [PATCH 00 of 10] x86: unify asm/pgtable.h

Ingo Molnar wrote:
> * Ingo Molnar <mingo@...e.hu> wrote:
>
>   
>>>>>>  #define __PAGE_KERNEL_EXEC						\
>>>>>> -	(_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_GLOBAL)
>>>>>> +	(_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACCESSED)
>>>>>>     
>>>>>>             
>>>> This shouldn't be necessary.  The old 64-bit code defined everything 
>>>> without _PAGE_GLOBAL, but then used a MAKE_GLOBAL() macro to OR it 
>>>> in later.  This seemed a bit roundabout to me, so I just put it in 
>>>> from the outset.
>>>>         
>> actually, this is wrong.
>>
>> a couple of places use __PAGE_* values, which you've now changed to 
>> include the _PAGE_GLOBAL flag.
>>     
>
> yep, fixing this resolves the crash.
>   

Bugger.  OK.

And I don't see quite how the global flag is causing the BUG bug in 
change_page_attr().  The logic is:

	if (pgprot_val(prot) != pgprot_val(ref_prot)) {
		...
	} else {
		if (!pte_huge(*kpte)) {
			...
		} else
			BUG();
	}

Is _PAGE_GLOBAL causing the first if() to fall through to the second 
clause?  Because otherwise it shouldn't have any effect on the 
pte_huge() test.

But given that ref_prot is set to PAGE_KERNEL or PAGE_KERNEL_EXEC, which 
will have _PAGE_GLOBAL in it either way, I don't see where the problem 
is coming from.

Gah!  This can't be right!  I think the original change_page_attr() code 
is plain buggy.

The crash call chain is:

  [<ffffffff8021db68>] change_page_attr_addr+0x9e/0x119
  [<ffffffff8021d44f>] ioremap_change_attr+0x49/0x58
  [<ffffffff8021d626>] iounmap+0xbe/0xe0
...

ioremap_change_attr does:

		err = change_page_attr_addr(vaddr,npages,__pgprot(__PAGE_KERNEL|flags));

Now, in the current code (ie, before my patch), __PAGE_KERNEL doesn't 
have _PAGE_GLOBAL set, but PAGE_KERNEL does.  Therefore, 
change_page_attr_addr calls

	__change_page_attr(address, pfn, prot, PAGE_KERNEL);

which means:

	__change_page_attr(address, pfn, pgprot(__PAGE_KERNEL), PAGE_KERNEL);

(iounmap always passes flags of 0) which just happens to fail the test:

	if (pgprot_val(prot) != pgprot_val(ref_prot)) {

because prot doesn't contain _PAGE_GLOBAL and ref_prot does.

In other words, prot and ref_prot can never be equal, so this path is 
always taken, and the other branch which tests pte_huge() is never run.

Andi?  Jan?  Is this code just buggy, or is there something else going 
on here?

    J
--
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/