Re: [PATCH 17/30] x86, kaiser: map debug IDT tables

[Dave Hansen]

On 11/20/2017 12:40 PM, Thomas Gleixner wrote:
> On Fri, 10 Nov 2017, Dave Hansen wrote:
>>  
>> +static int kaiser_user_map_ptr_early(const void *start_addr, unsigned long 
>> size,
>> + unsigned long flags)
>> +{
>> +int ret = kaiser_add_user_map(start_addr, size, flags);
>> +WARN_ON(ret);
>> +return ret;
> What's the point of the return value when it is ignored at the call site?

I'm dropping this patch, btw.  It was unnecessary.

Re: [PATCH 17/30] x86, kaiser: map debug IDT tables

[Dave Hansen]

On 11/20/2017 12:40 PM, Thomas Gleixner wrote:
> On Fri, 10 Nov 2017, Dave Hansen wrote:
>>  
>> +static int kaiser_user_map_ptr_early(const void *start_addr, unsigned long 
>> size,
>> + unsigned long flags)
>> +{
>> +int ret = kaiser_add_user_map(start_addr, size, flags);
>> +WARN_ON(ret);
>> +return ret;
> What's the point of the return value when it is ignored at the call site?

I'm dropping this patch, btw.  It was unnecessary.

Re: [PATCH 17/30] x86, kaiser: map debug IDT tables

[Thomas Gleixner]

On Mon, 20 Nov 2017, Andy Lutomirski wrote:

> On Fri, Nov 10, 2017 at 11:31 AM, Dave Hansen
>  wrote:
> >
> > From: Dave Hansen 
> >
> > The IDT is another structure which the CPU references via a
> > virtual address.  It also obviously needs these to handle an
> > interrupt in userspace, so these need to be mapped into the user
> > copy of the page tables.
> 
> Why would the debug IDT ever be used in user mode?  IIRC it's a total
> turd related to avoiding crap nesting inside NMI.  Or am I wrong?

No. It's called from the TRACE_IRQS macros in the ASM entry code and from
do_nmi().

> If it *is* used in user mode, then we have a bug and it should be in
> the IDT to avoid address leaks just like the normal IDT.

It's not so this can go away. Good catch.

Thanks,

tglx

Re: [PATCH 17/30] x86, kaiser: map debug IDT tables

[Thomas Gleixner]

On Mon, 20 Nov 2017, Andy Lutomirski wrote:

> On Fri, Nov 10, 2017 at 11:31 AM, Dave Hansen
>  wrote:
> >
> > From: Dave Hansen 
> >
> > The IDT is another structure which the CPU references via a
> > virtual address.  It also obviously needs these to handle an
> > interrupt in userspace, so these need to be mapped into the user
> > copy of the page tables.
> 
> Why would the debug IDT ever be used in user mode?  IIRC it's a total
> turd related to avoiding crap nesting inside NMI.  Or am I wrong?

No. It's called from the TRACE_IRQS macros in the ASM entry code and from
do_nmi().

> If it *is* used in user mode, then we have a bug and it should be in
> the IDT to avoid address leaks just like the normal IDT.

It's not so this can go away. Good catch.

Thanks,

tglx

Re: [PATCH 17/30] x86, kaiser: map debug IDT tables

[Andy Lutomirski]

On Fri, Nov 10, 2017 at 11:31 AM, Dave Hansen
 wrote:
>
> From: Dave Hansen 
>
> The IDT is another structure which the CPU references via a
> virtual address.  It also obviously needs these to handle an
> interrupt in userspace, so these need to be mapped into the user
> copy of the page tables.

Why would the debug IDT ever be used in user mode?  IIRC it's a total
turd related to avoiding crap nesting inside NMI.  Or am I wrong?

If it *is* used in user mode, then we have a bug and it should be in
the IDT to avoid address leaks just like the normal IDT.

--Andy

Re: [PATCH 17/30] x86, kaiser: map debug IDT tables

[Andy Lutomirski]

On Fri, Nov 10, 2017 at 11:31 AM, Dave Hansen
 wrote:
>
> From: Dave Hansen 
>
> The IDT is another structure which the CPU references via a
> virtual address.  It also obviously needs these to handle an
> interrupt in userspace, so these need to be mapped into the user
> copy of the page tables.

Why would the debug IDT ever be used in user mode?  IIRC it's a total
turd related to avoiding crap nesting inside NMI.  Or am I wrong?

If it *is* used in user mode, then we have a bug and it should be in
the IDT to avoid address leaks just like the normal IDT.

--Andy

Re: [PATCH 17/30] x86, kaiser: map debug IDT tables

[Thomas Gleixner]

On Fri, 10 Nov 2017, Dave Hansen wrote:
>  
> +static int kaiser_user_map_ptr_early(const void *start_addr, unsigned long 
> size,
> +  unsigned long flags)
> +{
> + int ret = kaiser_add_user_map(start_addr, size, flags);
> + WARN_ON(ret);
> + return ret;

What's the point of the return value when it is ignored at the call site?

> +}
> +
>  /*
>   * Ensure that the top level of the (shadow) page tables are
>   * entirely populated.  This ensures that all processes that get
> @@ -374,6 +382,10 @@ void __init kaiser_init(void)
> sizeof(gate_desc) * NR_VECTORS,
> __PAGE_KERNEL_RO | _PAGE_GLOBAL);
>  
> + kaiser_user_map_ptr_early(_idt_table,
> +   sizeof(gate_desc) * NR_VECTORS,
> +   __PAGE_KERNEL | _PAGE_GLOBAL);
> +

Thanks,

tglx

Re: [PATCH 17/30] x86, kaiser: map debug IDT tables

[Thomas Gleixner]

On Fri, 10 Nov 2017, Dave Hansen wrote:
>  
> +static int kaiser_user_map_ptr_early(const void *start_addr, unsigned long 
> size,
> +  unsigned long flags)
> +{
> + int ret = kaiser_add_user_map(start_addr, size, flags);
> + WARN_ON(ret);
> + return ret;

What's the point of the return value when it is ignored at the call site?

> +}
> +
>  /*
>   * Ensure that the top level of the (shadow) page tables are
>   * entirely populated.  This ensures that all processes that get
> @@ -374,6 +382,10 @@ void __init kaiser_init(void)
> sizeof(gate_desc) * NR_VECTORS,
> __PAGE_KERNEL_RO | _PAGE_GLOBAL);
>  
> + kaiser_user_map_ptr_early(_idt_table,
> +   sizeof(gate_desc) * NR_VECTORS,
> +   __PAGE_KERNEL | _PAGE_GLOBAL);
> +

Thanks,

tglx

[PATCH 17/30] x86, kaiser: map debug IDT tables

[Dave Hansen]

From: Dave Hansen 

The IDT is another structure which the CPU references via a
virtual address.  It also obviously needs these to handle an
interrupt in userspace, so these need to be mapped into the user
copy of the page tables.

Signed-off-by: Dave Hansen 
Cc: Moritz Lipp 
Cc: Daniel Gruss 
Cc: Michael Schwarz 
Cc: Richard Fellner 
Cc: Andy Lutomirski 
Cc: Linus Torvalds 
Cc: Kees Cook 
Cc: Hugh Dickins 
Cc: x...@kernel.org
---

 b/arch/x86/mm/kaiser.c |   12 
 1 file changed, 12 insertions(+)

diff -puN arch/x86/mm/kaiser.c~kaiser-user-map-trace-and-debug-idt 
arch/x86/mm/kaiser.c
--- a/arch/x86/mm/kaiser.c~kaiser-user-map-trace-and-debug-idt  2017-11-10 
11:22:14.332244936 -0800
+++ b/arch/x86/mm/kaiser.c  2017-11-10 11:22:14.336244936 -0800
@@ -286,6 +286,14 @@ int kaiser_add_user_map_ptrs(const void
   flags);
 }
 
+static int kaiser_user_map_ptr_early(const void *start_addr, unsigned long 
size,
+unsigned long flags)
+{
+   int ret = kaiser_add_user_map(start_addr, size, flags);
+   WARN_ON(ret);
+   return ret;
+}
+
 /*
  * Ensure that the top level of the (shadow) page tables are
  * entirely populated.  This ensures that all processes that get
@@ -374,6 +382,10 @@ void __init kaiser_init(void)
  sizeof(gate_desc) * NR_VECTORS,
  __PAGE_KERNEL_RO | _PAGE_GLOBAL);
 
+   kaiser_user_map_ptr_early(_idt_table,
+ sizeof(gate_desc) * NR_VECTORS,
+ __PAGE_KERNEL | _PAGE_GLOBAL);
+
/*
 * We could theoretically do this in setup_fixmap_gdt().
 * But, we would need to rewrite the above page table
_

[PATCH 17/30] x86, kaiser: map debug IDT tables

[Dave Hansen]

From: Dave Hansen 

The IDT is another structure which the CPU references via a
virtual address.  It also obviously needs these to handle an
interrupt in userspace, so these need to be mapped into the user
copy of the page tables.

Signed-off-by: Dave Hansen 
Cc: Moritz Lipp 
Cc: Daniel Gruss 
Cc: Michael Schwarz 
Cc: Richard Fellner 
Cc: Andy Lutomirski 
Cc: Linus Torvalds 
Cc: Kees Cook 
Cc: Hugh Dickins 
Cc: x...@kernel.org
---

 b/arch/x86/mm/kaiser.c |   12 
 1 file changed, 12 insertions(+)

diff -puN arch/x86/mm/kaiser.c~kaiser-user-map-trace-and-debug-idt 
arch/x86/mm/kaiser.c
--- a/arch/x86/mm/kaiser.c~kaiser-user-map-trace-and-debug-idt  2017-11-10 
11:22:14.332244936 -0800
+++ b/arch/x86/mm/kaiser.c  2017-11-10 11:22:14.336244936 -0800
@@ -286,6 +286,14 @@ int kaiser_add_user_map_ptrs(const void
   flags);
 }
 
+static int kaiser_user_map_ptr_early(const void *start_addr, unsigned long 
size,
+unsigned long flags)
+{
+   int ret = kaiser_add_user_map(start_addr, size, flags);
+   WARN_ON(ret);
+   return ret;
+}
+
 /*
  * Ensure that the top level of the (shadow) page tables are
  * entirely populated.  This ensures that all processes that get
@@ -374,6 +382,10 @@ void __init kaiser_init(void)
  sizeof(gate_desc) * NR_VECTORS,
  __PAGE_KERNEL_RO | _PAGE_GLOBAL);
 
+   kaiser_user_map_ptr_early(_idt_table,
+ sizeof(gate_desc) * NR_VECTORS,
+ __PAGE_KERNEL | _PAGE_GLOBAL);
+
/*
 * We could theoretically do this in setup_fixmap_gdt().
 * But, we would need to rewrite the above page table
_

[PATCH 17/30] x86, kaiser: map debug IDT tables

[Dave Hansen]

From: Dave Hansen 

The IDT table it references are another structure where the
CPU references a virtual address.  It also obviously needs these
to handle an interrupt in userspace, so these need to be mapped into
the user copy of the page tables.

Signed-off-by: Dave Hansen 
Cc: Moritz Lipp 
Cc: Daniel Gruss 
Cc: Michael Schwarz 
Cc: Richard Fellner 
Cc: Andy Lutomirski 
Cc: Linus Torvalds 
Cc: Kees Cook 
Cc: Hugh Dickins 
Cc: x...@kernel.org
---

 b/arch/x86/mm/kaiser.c |   12 
 1 file changed, 12 insertions(+)

diff -puN arch/x86/mm/kaiser.c~kaiser-user-map-trace-and-debug-idt 
arch/x86/mm/kaiser.c
--- a/arch/x86/mm/kaiser.c~kaiser-user-map-trace-and-debug-idt  2017-11-08 
10:45:35.124681380 -0800
+++ b/arch/x86/mm/kaiser.c  2017-11-08 10:45:35.127681380 -0800
@@ -275,6 +275,14 @@ int kaiser_add_user_map_ptrs(const void
   flags);
 }
 
+static int kaiser_user_map_ptr_early(const void *start_addr, unsigned long 
size,
+unsigned long flags)
+{
+   int ret = kaiser_add_user_map(start_addr, size, flags);
+   WARN_ON(ret);
+   return ret;
+}
+
 /*
  * Ensure that the top level of the (shadow) page tables are
  * entirely populated.  This ensures that all processes that get
@@ -363,6 +371,10 @@ void __init kaiser_init(void)
  sizeof(gate_desc) * NR_VECTORS,
  __PAGE_KERNEL_RO | _PAGE_GLOBAL);
 
+   kaiser_user_map_ptr_early(_idt_table,
+ sizeof(gate_desc) * NR_VECTORS,
+ __PAGE_KERNEL | _PAGE_GLOBAL);
+
/*
 * We could theoretically do this in setup_fixmap_gdt().
 * But, we would need to rewrite the above page table
_

[PATCH 17/30] x86, kaiser: map debug IDT tables

[Dave Hansen]

From: Dave Hansen 

The IDT table it references are another structure where the
CPU references a virtual address.  It also obviously needs these
to handle an interrupt in userspace, so these need to be mapped into
the user copy of the page tables.

Signed-off-by: Dave Hansen 
Cc: Moritz Lipp 
Cc: Daniel Gruss 
Cc: Michael Schwarz 
Cc: Richard Fellner 
Cc: Andy Lutomirski 
Cc: Linus Torvalds 
Cc: Kees Cook 
Cc: Hugh Dickins 
Cc: x...@kernel.org
---

 b/arch/x86/mm/kaiser.c |   12 
 1 file changed, 12 insertions(+)

diff -puN arch/x86/mm/kaiser.c~kaiser-user-map-trace-and-debug-idt 
arch/x86/mm/kaiser.c
--- a/arch/x86/mm/kaiser.c~kaiser-user-map-trace-and-debug-idt  2017-11-08 
10:45:35.124681380 -0800
+++ b/arch/x86/mm/kaiser.c  2017-11-08 10:45:35.127681380 -0800
@@ -275,6 +275,14 @@ int kaiser_add_user_map_ptrs(const void
   flags);
 }
 
+static int kaiser_user_map_ptr_early(const void *start_addr, unsigned long 
size,
+unsigned long flags)
+{
+   int ret = kaiser_add_user_map(start_addr, size, flags);
+   WARN_ON(ret);
+   return ret;
+}
+
 /*
  * Ensure that the top level of the (shadow) page tables are
  * entirely populated.  This ensures that all processes that get
@@ -363,6 +371,10 @@ void __init kaiser_init(void)
  sizeof(gate_desc) * NR_VECTORS,
  __PAGE_KERNEL_RO | _PAGE_GLOBAL);
 
+   kaiser_user_map_ptr_early(_idt_table,
+ sizeof(gate_desc) * NR_VECTORS,
+ __PAGE_KERNEL | _PAGE_GLOBAL);
+
/*
 * We could theoretically do this in setup_fixmap_gdt().
 * But, we would need to rewrite the above page table
_