[PATCH 0/5] x86/pti: small fixes and cleanups for pti

[PATCH 0/5] x86/pti: small fixes and cleanups for pti

[Jiang Biao]

This is a short series of fixes and cleanups found when reading the
code, no functional changes.

Jiang Biao (5):
  x86/pti: check the return value of pti_user_pagetable_walk_p4d
  x86/pti: check the return value of pti_user_pagetable_walk_pmd
  x86/pti: make pti_set_kernel_image_nonglobal static
  x86/pti: warn for unknown pti boot options
  x86/pti: constify address parameters

 arch/x86/mm/pti.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

-- 
2.7.4

[PATCH 1/5] x86/pti: check the return value of pti_user_pagetable_walk_p4d

[Jiang Biao]

pti_user_pagetable_walk_p4d() may return NULL, we should check the
return value to avoid NULL pointer dereference.

Signed-off-by: Jiang Biao <jiang.biao2@zte.com.cn>
---
 arch/x86/mm/pti.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
index 4d418e7..be9e5bc 100644
--- a/arch/x86/mm/pti.c
+++ b/arch/x86/mm/pti.c
@@ -195,8 +195,10 @@ static p4d_t *pti_user_pagetable_walk_p4d(unsigned long address)
 static pmd_t *pti_user_pagetable_walk_pmd(unsigned long address)
 {
 	gfp_t gfp = (GFP_KERNEL | __GFP_NOTRACK | __GFP_ZERO);
-	p4d_t *p4d = pti_user_pagetable_walk_p4d(address);
 	pud_t *pud;
+	p4d_t *p4d = pti_user_pagetable_walk_p4d(address);
+	if (WARN_ON(!p4d))
+		return NULL;
 
 	BUILD_BUG_ON(p4d_large(*p4d) != 0);
 	if (p4d_none(*p4d)) {
@@ -354,6 +356,9 @@ static void __init pti_clone_p4d(unsigned long addr)
 	pgd_t *kernel_pgd;
 
 	user_p4d = pti_user_pagetable_walk_p4d(addr);
+	if (WARN_ON(!user_p4d))
+		return;
+	
 	kernel_pgd = pgd_offset_k(addr);
 	kernel_p4d = p4d_offset(kernel_pgd, addr);
 	*user_p4d = *kernel_p4d;
-- 
2.7.4

Re: [PATCH 1/5] x86/pti: check the return value of pti_user_pagetable_walk_p4d

[Dave Hansen]

On 07/15/2018 09:03 PM, Jiang Biao wrote:
> diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
> index 4d418e7..be9e5bc 100644
> --- a/arch/x86/mm/pti.c
> +++ b/arch/x86/mm/pti.c
> @@ -195,8 +195,10 @@ static p4d_t *pti_user_pagetable_walk_p4d(unsigned long address)
>  static pmd_t *pti_user_pagetable_walk_pmd(unsigned long address)
>  {
>  	gfp_t gfp = (GFP_KERNEL | __GFP_NOTRACK | __GFP_ZERO);
> -	p4d_t *p4d = pti_user_pagetable_walk_p4d(address);
>  	pud_t *pud;
> +	p4d_t *p4d = pti_user_pagetable_walk_p4d(address);
> +	if (WARN_ON(!p4d))
> +		return NULL;

First of all, I don't think we need the (new) warning here.
pti_user_pagetable_walk_p4d() only returns NULL if you try it on a
userspace _address_ or the page allocation fails.  It already warns on
the address case.

If you think the allocation path needs a warning, please do it as close
as possible to the _source_ of the warning (the failed allocation), not
in the caller.

This goes for all the variations on this that you have in the set.

[PATCH 2/5] x86/pti: check the return value of pti_user_pagetable_walk_pmd

[Jiang Biao]

pti_user_pagetable_walk_pmd() may return NULL, we should check the
return value in pti_user_pagetable_walk_pte() to avoid NULL pointer
dereference like it is checked in pti_clone_pmds().

Signed-off-by: Jiang Biao <jiang.biao2@zte.com.cn>
---
 arch/x86/mm/pti.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
index be9e5bc..bb6f608 100644
--- a/arch/x86/mm/pti.c
+++ b/arch/x86/mm/pti.c
@@ -239,8 +239,10 @@ static pmd_t *pti_user_pagetable_walk_pmd(unsigned long address)
 static __init pte_t *pti_user_pagetable_walk_pte(unsigned long address)
 {
 	gfp_t gfp = (GFP_KERNEL | __GFP_NOTRACK | __GFP_ZERO);
-	pmd_t *pmd = pti_user_pagetable_walk_pmd(address);
 	pte_t *pte;
+	pmd_t *pmd = pti_user_pagetable_walk_pmd(address);
+	if (WARN_ON(!pmd))
+		return NULL;
 
 	/* We can't do anything sensible if we hit a large mapping. */
 	if (pmd_large(*pmd)) {
-- 
2.7.4

[PATCH 3/5] x86/pti: make pti_set_kernel_image_nonglobal static

[Jiang Biao]

pti_set_kernel_image_nonglobal() is only used in pti.c, make it
static.

Signed-off-by: Jiang Biao <jiang.biao2@zte.com.cn>
---
 arch/x86/mm/pti.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
index bb6f608..a76b2cc 100644
--- a/arch/x86/mm/pti.c
+++ b/arch/x86/mm/pti.c
@@ -473,7 +473,7 @@ void pti_clone_kernel_text(void)
  * the other set_memory.h functions.  Just extern it.
  */
 extern int set_memory_nonglobal(unsigned long addr, int numpages);
-void pti_set_kernel_image_nonglobal(void)
+static void pti_set_kernel_image_nonglobal(void)
 {
 	/*
 	 * The identity map is created with PMDs, regardless of the
-- 
2.7.4

Re: [PATCH 3/5] x86/pti: make pti_set_kernel_image_nonglobal static

[Dave Hansen]

On 07/15/2018 09:03 PM, Jiang Biao wrote:
> pti_set_kernel_image_nonglobal() is only used in pti.c, make it
> static.

I went back and forth on where this gets called from so it makes sense
that this would get screwed up.  grep confirms it is not getting called
from elsewhere.

Acked-by: Dave Hansen <dave.hansen@intel.com>

[tip:x86/pti] x86/pti: Make pti_set_kernel_image_nonglobal() static

[tip-bot for Jiang Biao]

Commit-ID:  21279157efffe5e7258483809942d576cb802768
Gitweb:     https://git.kernel.org/tip/21279157efffe5e7258483809942d576cb802768
Author:     Jiang Biao <jiang.biao2@zte.com.cn>
AuthorDate: Mon, 16 Jul 2018 12:03:38 +0800
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Mon, 16 Jul 2018 17:59:57 +0200

x86/pti: Make pti_set_kernel_image_nonglobal() static

pti_set_kernel_image_nonglobal() is only used in pti.c, make it static.

Signed-off-by: Jiang Biao <jiang.biao2@zte.com.cn>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: luto@kernel.org
Cc: hpa@zytor.com
Cc: albcamus@gmail.com
Cc: zhong.weidong@zte.com.cn
Link: https://lkml.kernel.org/r/1531713820-24544-4-git-send-email-jiang.biao2@zte.com.cn

---
 arch/x86/mm/pti.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
index 4d418e705878..9277e9ba92b5 100644
--- a/arch/x86/mm/pti.c
+++ b/arch/x86/mm/pti.c
@@ -466,7 +466,7 @@ void pti_clone_kernel_text(void)
  * the other set_memory.h functions.  Just extern it.
  */
 extern int set_memory_nonglobal(unsigned long addr, int numpages);
-void pti_set_kernel_image_nonglobal(void)
+static void pti_set_kernel_image_nonglobal(void)
 {
 	/*
 	 * The identity map is created with PMDs, regardless of the

[PATCH 4/5] x86/pti: warn for unknown pti boot options

[Jiang Biao]

When using unknown pti boot options other than on/off/auto, we
select auto silently, which is sometimes confusing. Add warning for
unknown pti boot options like we do in
spectre_v2_select_mitigation().

Signed-off-by: Jiang Biao <jiang.biao2@zte.com.cn>
---
 arch/x86/mm/pti.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
index a76b2cc..a368656 100644
--- a/arch/x86/mm/pti.c
+++ b/arch/x86/mm/pti.c
@@ -92,14 +92,14 @@ void __init pti_check_boottime_disable(void)
 			pti_mode = PTI_FORCE_OFF;
 			pti_print_if_insecure("disabled on command line.");
 			return;
-		}
-		if (ret == 2 && !strncmp(arg, "on", 2)) {
+		} else if (ret == 2 && !strncmp(arg, "on", 2)) {
 			pti_mode = PTI_FORCE_ON;
 			pti_print_if_secure("force enabled on command line.");
 			goto enable;
-		}
-		if (ret == 4 && !strncmp(arg, "auto", 4)) {
-			pti_mode = PTI_AUTO;
+		} else if (ret == 4 && !strncmp(arg, "auto", 4)) {
+			goto autosel;
+		} else {
+			pr_err("unknown option (%s). Switching to AUTO select\n", arg);
 			goto autosel;
 		}
 	}
-- 
2.7.4

Re: [PATCH 4/5] x86/pti: warn for unknown pti boot options

[Dave Hansen]

On 07/15/2018 09:03 PM, Jiang Biao wrote:
> When using unknown pti boot options other than on/off/auto, we
> select auto silently, which is sometimes confusing. Add warning for
> unknown pti boot options like we do in
> spectre_v2_select_mitigation().

There's not a lot of precedent for this one.  We have gazillions of boot
options and I don't know of many places that we do this.  While this
isn't a horrible idea, I don't look forward to the patches will do this
for every other option in the kernel.

[PATCH 5/5] x86/pti: constify address parameters

[Jiang Biao]

Addresses passed in pti_user_pagetable_walk_*() are not supposed to
change at runtime, make them const to aviod future slipups.

Signed-off-by: Jiang Biao <jiang.biao2@zte.com.cn>
---
 arch/x86/mm/pti.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
index a368656..ac2cbdd 100644
--- a/arch/x86/mm/pti.c
+++ b/arch/x86/mm/pti.c
@@ -164,7 +164,7 @@ pgd_t __pti_set_user_pgd(pgd_t *pgdp, pgd_t pgd)
  *
  * Returns a pointer to a P4D on success, or NULL on failure.
  */
-static p4d_t *pti_user_pagetable_walk_p4d(unsigned long address)
+static p4d_t *pti_user_pagetable_walk_p4d(const unsigned long address)
 {
 	pgd_t *pgd = kernel_to_user_pgdp(pgd_offset_k(address));
 	gfp_t gfp = (GFP_KERNEL | __GFP_NOTRACK | __GFP_ZERO);
@@ -192,7 +192,7 @@ static p4d_t *pti_user_pagetable_walk_p4d(unsigned long address)
  *
  * Returns a pointer to a PMD on success, or NULL on failure.
  */
-static pmd_t *pti_user_pagetable_walk_pmd(unsigned long address)
+static pmd_t *pti_user_pagetable_walk_pmd(const unsigned long address)
 {
 	gfp_t gfp = (GFP_KERNEL | __GFP_NOTRACK | __GFP_ZERO);
 	pud_t *pud;
@@ -236,7 +236,7 @@ static pmd_t *pti_user_pagetable_walk_pmd(unsigned long address)
  *
  * Returns a pointer to a PTE on success, or NULL on failure.
  */
-static __init pte_t *pti_user_pagetable_walk_pte(unsigned long address)
+static __init pte_t *pti_user_pagetable_walk_pte(const unsigned long address)
 {
 	gfp_t gfp = (GFP_KERNEL | __GFP_NOTRACK | __GFP_ZERO);
 	pte_t *pte;
-- 
2.7.4

Re: [PATCH 5/5] x86/pti: constify address parameters

[Dave Hansen]

On 07/15/2018 09:03 PM, Jiang Biao wrote:
> -static p4d_t *pti_user_pagetable_walk_p4d(unsigned long address)
> +static p4d_t *pti_user_pagetable_walk_p4d(const unsigned long address)
>  {
>  	pgd_t *pgd = kernel_to_user_pgdp(pgd_offset_k(address));
>  	gfp_t gfp = (GFP_KERNEL | __GFP_NOTRACK | __GFP_ZERO);

I'm also a bit ambivalent on this one.  While this is _correct_ and not
something I think we actively discourage, it's not something that we're
horribly diligent about doing universally in the kernel.