Re: [RFC PATCH] x86/mm: Mask out unsupported bit when it set noexec=off

[Thomas Gleixner <tglx@linutronix.de>]

Zhong,

On Mon, 11 Nov 2019, zhong jiang wrote:

> Commit 510bb96fe5b3 ("x86/mm: Prevent bogus warnings with "noexec=off"")
> use  __supported_pte_mask to replace __default_kernel_pte_mask to mask
> out the unsupported bits. It works when the command line set noexec=off.
> 
> It also seems to works to use __supported_pte_mask instead in native_set_fixmap.

"Seems to work" is really not a good engineering principle and neither a
good rationale WHY this change is correct, which it is not.

> @@ -647,7 +647,7 @@ void native_set_fixmap(unsigned /* enum fixed_addresses */ idx,
>  		       phys_addr_t phys, pgprot_t flags)
>  {
>  	/* Sanitize 'prot' against any unsupported bits: */
> -	pgprot_val(flags) &= __default_kernel_pte_mask;
> +	pgprot_val(flags) &= __supported_pte_mask;
>  
>  	__native_set_fixmap(idx, pfn_pte(phys >> PAGE_SHIFT, flags));
>  }

The commit you mentioned switched __early_set_fixmap() to
__supported_pte_mask because __default_kernel_pte_mask is not yet set up at
that point which caused the following warning:

     attempted to set unsupported pgprot:    8000000000000163
                                  bits:      8000000000000000
                                  supported: 7fffffffffffffff

At this point __default_kernel_pte_mask is still compile time initialized
to ~0UL, i.e. all bits set which allows the NX bit to be set, but it's not
supported according to __supported_pte_mask.

Once __default_kernel_pte_mask is properly runtime initialized to:

     __default_kernel_pte_mask = __supported_pte_mask;

in probe_page_size_mask() there is no reason that subsequent code uses
__supported_pte_mask.

In fact that's just wrong because __default_kernel_pte_mask can have extra
bits cleared during runtime initialization, e.g.:

        /* Except when with PTI where the kernel is mostly non-Global: */
        if (cpu_feature_enabled(X86_FEATURE_PTI))
                __default_kernel_pte_mask &= ~_PAGE_GLOBAL;

So your change "works", but subtly breaks PTI protections.

Please be more careful next time and really analyse why your change is
correct and provide this analysis in the changelog.

Thanks,

	tglx