linux-kernel - Re: x86/mm/pageattr: Code without effect?

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: <20130405142104.GB29290@pd.tnic>
Date:	Fri, 5 Apr 2013 16:21:04 +0200
From:	Borislav Petkov <bp@...en8.de>
To:	Stefan Bader <stefan.bader@...onical.com>,
	Andrea Arcangeli <aarcange@...hat.com>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Ingo Molnar <mingo@...nel.org>,
	Andy Whitcroft <apw@...onical.com>,
	Mel Gorman <mgorman@...e.de>
Subject: Re: x86/mm/pageattr: Code without effect?

On Fri, Apr 05, 2013 at 11:01:02AM +0200, Stefan Bader wrote:
> When looking through some mm code I stumbled over one part in
> arch/x86/mm/pageattr.c that looks somewhat bogus to me. Cannot
> say what exactly the effects are, but maybe you do (or you could
> explain to me why I am wrong :)).
> 
> commit a8aed3e0752b4beb2e37cbed6df69faae88268da
> Author: Andrea Arcangeli <aarcange@...hat.com>
> Date:   Fri Feb 22 15:11:51 2013 -0800
> 
>     x86/mm/pageattr: Prevent PSE and GLOABL leftovers to confuse
>     pmd/pte_present and pmd_huge
> 
> added the following to try_preserve_large_page:
> 
>  	/*
> +	 * Set the PSE and GLOBAL flags only if the PRESENT flag is
> +	 * set otherwise pmd_present/pmd_huge will return true even on
> +	 * a non present pmd. The canon_pgprot will clear _PAGE_GLOBAL
> +	 * for the ancient hardware that doesn't support it.
> +	 */
> +	if (pgprot_val(new_prot) & _PAGE_PRESENT)
> +		pgprot_val(new_prot) |= _PAGE_PSE | _PAGE_GLOBAL;
> +	else
> +		pgprot_val(new_prot) &= ~(_PAGE_PSE | _PAGE_GLOBAL);
> +
> +	new_prot = canon_pgprot(new_prot);
> +
> +	/*
> 
> but (extending what follows after the changes)
> 
> 	 * old_pte points to the large page base address. So we need
> 	 * to add the offset of the virtual address:
> 	 */
> 	pfn = pte_pfn(old_pte) + ((address & (psize - 1)) >> PAGE_SHIFT);
> 	cpa->pfn = pfn;
> 
> 	new_prot = static_protections(req_prot, address, pfn);
> 
> So new_prot gets completely replaced by req_prot and all changes done to
> new_prot before look to be lost (the PSE and GLOBAL bit settings as well
> as the canon_pgprot call.
> 
> Maybe the hunk is useless anyway, or the breakage is subtle, or I miss something...

Yeah, I had to unwillingly stare at this crazy code recently too and
I can share your confusion. And from trying to grok what's going
on, I *think* what we actually meant to do is sanitize our required
protections first, i.e.

	new_prot = static_protections(req_prot, address, pfn);

and *then* do the _PAGE_PRESENT massaging. It does at least make sense
that way. And this is what we already do in __change_page_attr() for a
4K pte.

Andrea?

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
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/