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

2017-11-21 Thread 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

2017-11-21 Thread 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

2017-11-20 Thread 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

2017-11-20 Thread 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

2017-11-20 Thread 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

2017-11-20 Thread 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

2017-11-20 Thread 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

2017-11-20 Thread 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