Re: [PATCH 2/2] x86/mm/pti: warn and stop when pti_clone_pagetable() is on 1G page

From: Dave Hansen <dave.hansen@intel.com>
To: Lai Jiangshan <jiangshanlai@gmail.com>, linux-kernel@vger.kernel.org
Cc: Lai Jiangshan <laijs@linux.alibaba.com>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Andy Lutomirski <luto@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>
Subject: Re: [PATCH 2/2] x86/mm/pti: warn and stop when pti_clone_pagetable() is on 1G page
Date: Mon, 30 Nov 2020 08:37:15 -0800	[thread overview]
Message-ID: <5f3e903d-0cd8-c94a-a2f5-9238ee5efc43@intel.com> (raw)
In-Reply-To: <20201130152516.2387-2-jiangshanlai@gmail.com>

On 11/30/20 7:25 AM, Lai Jiangshan wrote:
> --- a/arch/x86/mm/pti.c
> +++ b/arch/x86/mm/pti.c
> @@ -321,10 +321,10 @@ pti_clone_pgtable(unsigned long start, unsigned long end,
>  			break;
>  
>  		pgd = pgd_offset_k(addr);
> -		if (WARN_ON(pgd_none(*pgd)))
> +		if (WARN_ON(pgd_none(*pgd) || pgd_large(*pgd)))
>  			return;
>  		p4d = p4d_offset(pgd, addr);
> -		if (WARN_ON(p4d_none(*p4d)))
> +		if (WARN_ON(p4d_none(*p4d) || p4d_large(*p4d)))
>  			return;
>  
>  		pud = pud_offset(p4d, addr);
> @@ -333,6 +333,8 @@ pti_clone_pgtable(unsigned long start, unsigned long end,
>  			addr = round_up(addr + 1, PUD_SIZE);
>  			continue;
>  		}
> +		if (WARN_ON(pud_large(*pud)))
> +			return;

One bit of practical application missing from the changelog: Right now,
we only clone parts of the kernel image and the cpu entry area.  The cpu
entry area would be insane to map with 1G pages since it maps so many
different kinds of pages and has *small* mappings.

For the kernel image to have a 1GB area with uniform permissions seems
pretty far away to me.  It would be en even more remote possibility that
a large swath of it would need to be cloned for PTI.  Kernel text with a
non-PCID system is probably as close as we would get.  I'm also not even
sure we have the code to create 1GB mappings for parts of the image.

While I'm fine with this for robustness and self-documentation, I think
there needs to be a bit more on this in the changelog.

Also, wouldn't we be better off if we added warnings to the p*d_offset()
functions?  The real problem here, for instance, is passing a
pgd_large()==1 pgd to p4d_offset().