linux-kernel - Re: [PATCH 1/2] x86/mm/pat: Change pat

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: <20160311091229.GA4347@pd.tnic>
Date:	Fri, 11 Mar 2016 10:12:30 +0100
From:	Borislav Petkov <bp@...en8.de>
To:	Toshi Kani <toshi.kani@....com>
Cc:	mingo@...nel.org, bp@...e.de, hpa@...or.com, tglx@...utronix.de,
	mcgrof@...e.com, jgross@...e.com, paul.gortmaker@...driver.com,
	x86@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table

On Thu, Mar 10, 2016 at 09:45:45PM -0700, Toshi Kani wrote:
> Since 'commit 9cd25aac1f44 ("x86/mm/pat: Emulate PAT when it
> is disabled")', we emulate a PAT table when PAT is disabled.
> This requires pat_init() be called even if PAT is disabled,
> which revealed a long standing issue that PAT is left enabled
> without calling pat_init() at all.
> 
> pat_init() is called from MTRR code since it relies on MTRR's
> rendezvous handler to initialize PAT for all APs .  However,
> when CPU does not support MTRR, ex. qemu32's virtual CPU, MTRR
> is set disabled and does not call pat_init().  There is no
> interface available for MTRR to disable PAT, either.
> 
> Change pat_disable() to a regular function (from an inline func)
> so that MTRR can call it to disable PAT when MTRR is disabled.
> pat_disable() sets PAT disabled, and calls pat_disable_init()
> to emulate the PAT table.
> 
> link: https://lkml.org/lkml/2016/3/10/402
> Reported-by: Paul Gortmaker <paul.gortmaker@...driver.com>
> Signed-off-by: Toshi Kani <toshi.kani@....com>
> Cc: Borislav Petkov <bp@...e.de>
> Cc: Luis R. Rodriguez <mcgrof@...e.com>
> Cc: Juergen Gross <jgross@...e.com>
> Cc: Ingo Molnar <mingo@...nel.org>
> Cc: H. Peter Anvin <hpa@...or.com>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> ---
>  arch/x86/include/asm/pat.h |    1 +
>  arch/x86/mm/pat.c          |   84 +++++++++++++++++++++++++++-----------------
>  2 files changed, 52 insertions(+), 33 deletions(-)
> 
> diff --git a/arch/x86/include/asm/pat.h b/arch/x86/include/asm/pat.h
> index ca6c228..016142b 100644
> --- a/arch/x86/include/asm/pat.h
> +++ b/arch/x86/include/asm/pat.h
> @@ -5,6 +5,7 @@
>  #include <asm/pgtable_types.h>
>  
>  bool pat_enabled(void);
> +void pat_disable(const char *reason);
>  extern void pat_init(void);
>  void pat_init_cache_modes(u64);
>  
> diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
> index f4ae536..1ff8aa9 100644
> --- a/arch/x86/mm/pat.c
> +++ b/arch/x86/mm/pat.c
> @@ -40,11 +40,19 @@
>  static bool boot_cpu_done;
>  
>  static int __read_mostly __pat_enabled = IS_ENABLED(CONFIG_X86_PAT);
> +static void pat_disable_init(void);
>  
> -static inline void pat_disable(const char *reason)
> +void pat_disable(const char *reason)
>  {
> +	if (boot_cpu_done) {
> +		pr_info("x86/PAT: PAT cannot be disabled after initialized\n");

		pr_err()

> +		return;
> +	}
> +
>  	__pat_enabled = 0;
>  	pr_info("x86/PAT: %s\n", reason);
> +
> +	pat_disable_init();

Why can't you call pat_init() here simply? It checks pat_enabled(). You
can call it pat_setup() or so if it looks confusing to call an init
function in a disable function...

Then you don't have to add yet another static disable_init_done but rely on
boot_cpu_done which gets set in pat_init().

Also, I don't see the static_cpu_has() check I suggested yesterday - we need to
check the feature bits if PAT gets disabled early on some old Intels.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.