linux-kernel - Re: [PATCH V8 08/44] x86/fault: Adjust WARN

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: <01a47dd8-4717-7b07-39ad-45fee1f78311@intel.com>
Date:   Fri, 28 Jan 2022 15:10:24 -0800
From:   Dave Hansen <dave.hansen@...el.com>
To:     ira.weiny@...el.com, Dave Hansen <dave.hansen@...ux.intel.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Dan Williams <dan.j.williams@...el.com>
Cc:     Fenghua Yu <fenghua.yu@...el.com>,
        Rick Edgecombe <rick.p.edgecombe@...el.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH V8 08/44] x86/fault: Adjust WARN_ON for PKey fault

On 1/27/22 09:54, ira.weiny@...el.com wrote:
> From: Ira Weiny <ira.weiny@...el.com>
> 
> Previously if a Protection key fault occurred it indicated something
> very wrong because user page mappings are not supposed to be in the
> kernel address space.

This is missing a key point.  The problem is PK faults on "*kernel*
addresses.

> Now PKey faults may happen on kernel mappings if the feature is enabled.

One nit: I've been using "pkeys" and "pkey" as the terms.  I usually
don't capitalize them except at the beginning of a sentence.

> If PKS is enabled, avoid the warning in the fault path.
> 
> Cc: Sean Christopherson <seanjc@...gle.com>
> Cc: Dan Williams <dan.j.williams@...el.com>
> Signed-off-by: Ira Weiny <ira.weiny@...el.com>
> ---
>  arch/x86/mm/fault.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index d0074c6ed31a..6ed91b632eac 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -1148,11 +1148,15 @@ do_kern_addr_fault(struct pt_regs *regs, unsigned long hw_error_code,
>  		   unsigned long address)
>  {
>  	/*
> -	 * Protection keys exceptions only happen on user pages.  We
> -	 * have no user pages in the kernel portion of the address
> -	 * space, so do not expect them here.
> +	 * X86_PF_PK (Protection key exceptions) may occur on kernel addresses
> +	 * when PKS (PKeys Supervisor) is enabled.
> +	 *
> +	 * However, if PKS is not enabled WARN if this exception is seen
> +	 * because there are no user pages in the kernel portion of the address
> +	 * space.
>  	 */
> -	WARN_ON_ONCE(hw_error_code & X86_PF_PK);
> +	WARN_ON_ONCE(!cpu_feature_enabled(X86_FEATURE_PKS) &&
> +		     (hw_error_code & X86_PF_PK));
>  
>  #ifdef CONFIG_X86_32
>  	/*

I'm wondering if this warning is even doing us any good.  I'm pretty
sure it's never triggered on me at least.  Either way, let's not get too
carried away with the comment.  I think this should do:

	/*
	 * PF_PF faults should only occur on kernel
	 * addresses when supervisor pkeys are enabled.
	 */