[PATCH] x86: use the correct function type for native_set_fixmap

[PATCH] x86: use the correct function type for native_set_fixmap

[Sami Tolvanen]

We call native_set_fixmap indirectly through the function pointer
struct pv_mmu_ops::set_fixmap, which expects the first parameter to be
'unsigned' instead of 'enum fixed_addresses'. This patch changes the
function type for native_set_fixmap to match the pointer, which fixes
indirect call mismatches with Control-Flow Integrity (CFI) checking.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 arch/x86/include/asm/fixmap.h | 2 +-
 arch/x86/mm/pgtable.c         | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/fixmap.h b/arch/x86/include/asm/fixmap.h
index 9da8cccdf3fb..6a295acd3de6 100644
--- a/arch/x86/include/asm/fixmap.h
+++ b/arch/x86/include/asm/fixmap.h
@@ -157,7 +157,7 @@ extern pte_t *kmap_pte;
 extern pte_t *pkmap_page_table;
 
 void __native_set_fixmap(enum fixed_addresses idx, pte_t pte);
-void native_set_fixmap(enum fixed_addresses idx,
+void native_set_fixmap(unsigned /* enum fixed_addresses */ idx,
 		       phys_addr_t phys, pgprot_t flags);
 
 #ifndef CONFIG_PARAVIRT_XXL
diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
index 44816ff6411f..d0ad35e3de74 100644
--- a/arch/x86/mm/pgtable.c
+++ b/arch/x86/mm/pgtable.c
@@ -647,8 +647,8 @@ void __native_set_fixmap(enum fixed_addresses idx, pte_t pte)
 	fixmaps_set++;
 }
 
-void native_set_fixmap(enum fixed_addresses idx, phys_addr_t phys,
-		       pgprot_t flags)
+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;
-- 
2.23.0.237.gc6a4ce50a0-goog

Re: [PATCH] x86: use the correct function type for native_set_fixmap

[Kees Cook]

On Fri, Sep 13, 2019 at 02:14:02PM -0700, Sami Tolvanen wrote:
> We call native_set_fixmap indirectly through the function pointer
> struct pv_mmu_ops::set_fixmap, which expects the first parameter to be
> 'unsigned' instead of 'enum fixed_addresses'. This patch changes the
> function type for native_set_fixmap to match the pointer, which fixes
> indirect call mismatches with Control-Flow Integrity (CFI) checking.
> 
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>

Is it correct that pv_mmu_ops can't be changed since this is an external
API?

Assuming so, then:

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  arch/x86/include/asm/fixmap.h | 2 +-
>  arch/x86/mm/pgtable.c         | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/include/asm/fixmap.h b/arch/x86/include/asm/fixmap.h
> index 9da8cccdf3fb..6a295acd3de6 100644
> --- a/arch/x86/include/asm/fixmap.h
> +++ b/arch/x86/include/asm/fixmap.h
> @@ -157,7 +157,7 @@ extern pte_t *kmap_pte;
>  extern pte_t *pkmap_page_table;
>  
>  void __native_set_fixmap(enum fixed_addresses idx, pte_t pte);
> -void native_set_fixmap(enum fixed_addresses idx,
> +void native_set_fixmap(unsigned /* enum fixed_addresses */ idx,
>  		       phys_addr_t phys, pgprot_t flags);
>  
>  #ifndef CONFIG_PARAVIRT_XXL
> diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
> index 44816ff6411f..d0ad35e3de74 100644
> --- a/arch/x86/mm/pgtable.c
> +++ b/arch/x86/mm/pgtable.c
> @@ -647,8 +647,8 @@ void __native_set_fixmap(enum fixed_addresses idx, pte_t pte)
>  	fixmaps_set++;
>  }
>  
> -void native_set_fixmap(enum fixed_addresses idx, phys_addr_t phys,
> -		       pgprot_t flags)
> +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;
> -- 
> 2.23.0.237.gc6a4ce50a0-goog
> 

-- 
Kees Cook

Re: [PATCH] x86: use the correct function type for native_set_fixmap

[Sami Tolvanen]

On Mon, Sep 23, 2019 at 4:28 PM Kees Cook <keescook@chromium.org> wrote:
> Is it correct that pv_mmu_ops can't be changed since this is an external
> API?

Someone else probably knows better, but yes, we could also fix this by
changing set_fixmap to accept enum fixed_addresses as the first
parameter, and changing the type of xen_set_fixmap instead.

Sami

Re: [PATCH] x86: use the correct function type for native_set_fixmap

[Sami Tolvanen]

On Tue, Sep 24, 2019 at 12:51 PM Sami Tolvanen <samitolvanen@google.com> wrote:
> Someone else probably knows better, but yes, we could also fix this by
> changing set_fixmap to accept enum fixed_addresses as the first
> parameter, and changing the type of xen_set_fixmap instead.

This approach is actually more problematic since we cannot forward
declare an enum in C and including <asm/fixmap.h> here results in a
ton of other missing dependencies. I assume this is why the callback
function accepts unsigned in the first place.

Sami

[tip: x86/entry] x86/mm: Use the correct function type for native_set_fixmap()

[tip-bot2 for Sami Tolvanen]

The following commit has been merged into the x86/entry branch of tip:

Commit-ID:     f53e2cd0b8ab7d9e390414470bdbd830f660133f
Gitweb:        https://git.kernel.org/tip/f53e2cd0b8ab7d9e390414470bdbd830f660133f
Author:        Sami Tolvanen <samitolvanen@google.com>
AuthorDate:    Fri, 13 Sep 2019 14:14:02 -07:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Fri, 11 Oct 2019 12:52:32 +02:00

x86/mm: Use the correct function type for native_set_fixmap()

We call native_set_fixmap indirectly through the function pointer
struct pv_mmu_ops::set_fixmap, which expects the first parameter to be
'unsigned' instead of 'enum fixed_addresses'. This patch changes the
function type for native_set_fixmap to match the pointer, which fixes
indirect call mismatches with Control-Flow Integrity (CFI) checking.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: H . Peter Anvin <hpa@zytor.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@surriel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: https://lkml.kernel.org/r/20190913211402.193018-1-samitolvanen@google.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/fixmap.h | 2 +-
 arch/x86/mm/pgtable.c         | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/fixmap.h b/arch/x86/include/asm/fixmap.h
index 0c47aa8..28183ee 100644
--- a/arch/x86/include/asm/fixmap.h
+++ b/arch/x86/include/asm/fixmap.h
@@ -156,7 +156,7 @@ extern pte_t *kmap_pte;
 extern pte_t *pkmap_page_table;
 
 void __native_set_fixmap(enum fixed_addresses idx, pte_t pte);
-void native_set_fixmap(enum fixed_addresses idx,
+void native_set_fixmap(unsigned /* enum fixed_addresses */ idx,
 		       phys_addr_t phys, pgprot_t flags);
 
 #ifndef CONFIG_PARAVIRT_XXL
diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
index 3e4b903..7bd2c3a 100644
--- a/arch/x86/mm/pgtable.c
+++ b/arch/x86/mm/pgtable.c
@@ -643,8 +643,8 @@ void __native_set_fixmap(enum fixed_addresses idx, pte_t pte)
 	fixmaps_set++;
 }
 
-void native_set_fixmap(enum fixed_addresses idx, phys_addr_t phys,
-		       pgprot_t flags)
+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;