linux-kernel - Re: [PATCH v3 3/5] x86/mm: add help function to check specific protection flags in range

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: <alpine.DEB.2.21.1809032357430.1462@nanos.tec.linutronix.de>
Date:   Tue, 4 Sep 2018 00:10:32 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Bin Yang <bin.yang@...el.com>
cc:     mingo@...nel.org, hpa@...or.com, x86@...nel.org,
        linux-kernel@...r.kernel.org, peterz@...radead.org,
        dave.hansen@...el.com, mark.gross@...el.com
Subject: Re: [PATCH v3 3/5] x86/mm: add help function to check specific
 protection flags in range

On Tue, 21 Aug 2018, Bin Yang wrote:
>  /*
> + * static_protections() "forces" page protections for some address
> + * ranges.  Return true if any part of the address/len range is forced
> + * to change from 'prot'.
> + */
> +static inline bool
> +needs_static_protections(pgprot_t prot, unsigned long address,
> +		unsigned long len, unsigned long pfn)
> +{
> +	int i;
> +
> +	address &= PAGE_MASK;
> +	len = PFN_ALIGN(len);
> +	for (i = 0; i < (len >> PAGE_SHIFT); i++, address += PAGE_SIZE, pfn++) {
> +		pgprot_t chk_prot = static_protections(prot, address, pfn);
> +
> +		if (pgprot_val(chk_prot) != pgprot_val(prot))
> +			return true;
> +	}
> +
> +	/* Does static_protections() demand a change ? */
> +	return false;
> +}

...

>  	if (cpa->force_split)
> @@ -660,14 +684,8 @@ try_preserve_large_page(pte_t *kpte, unsigned long address,
>  	 * static_protection() requires a different pgprot for one of
>  	 * the pages in the range we try to preserve:
>  	 */
> -	pfn = old_pfn;
> -	for (i = 0; i < (psize >> PAGE_SHIFT); i++, addr += PAGE_SIZE, pfn++) {
> -		pgprot_t chk_prot = static_protections(req_prot, addr, pfn);
> -
> -		if (pgprot_val(chk_prot) != pgprot_val(new_prot))
> -			goto out_unlock;
> -	}
> -
> +	if (needs_static_protections(new_prot, addr, psize, old_pfn))
> +		goto out_unlock;

This is not the same. The existing code does:

     new_prot = static_protections(req_prot, address, pfn);

which is the protection updated pgprot for the base of the address range
which should be modified. The loop does:

    chk_prot = static_protections(req_prot, addr, pfn);

    if (chk_prot != new_prot)
    	   goto split;

Now mapping your new function back and then the loop becomes:

    chk_prot = static_protections(new_prot, addr, pfn);

    if (chk_prot != new_prot)
    	   goto split;

which is broken in case that after the initial static protections
invocation

	new_prot = static_protections(req_prot, address, pfn);

the result is:

   new_prot != req_prot

and in the loop

   new_prot is valid for _ALL_ pages in the large page because the static
   protection which got applied for the first address can be applied to the
   complete range, i.e. new_prot it is not further modified by
   static_protections() for any page.

That again can cause wrong large page preservations.

Thanks,

	tglx