RE: [PATCH v13 3/3] trace,x86: Add irq vector tracepoints

[Seiji Aguchi]

> Yeah, I believe this does work. But you probably should add a comment
> like the following:

OK. I will add  some  comment above " extern atomic_long_t 
current_idt_descr_ptr;".

> 
> /*
>  * The current_idt_descr_ptr can only be set out of interrupt context
>  * to avoid races. 

I will introduce set_current_idt() as follows.

set_current_idt(unsigned long idt)
{
        If (WARN_ON_ONCE(in_interrupt()))
                return;

        atomic_long_set(idt);

}


> * Once set, the load_current_idt() is called by interrupt
>  * context either by NMI, debug, or via a smp_call_function(). That way
>  * the IDT will always be set back to the expected descriptor.
>  */

The important thing is not "called by interrupt context" but "called with 
interrupt disabled"
to avoid races.
Actually, load_current_idt() is called in process context in 
irq_vector_{reg/unreg}func().
In next patch, I will rewrite the comment.

> >
> > +extern atomic_long_t current_idt_descr_ptr;
> > +static inline void load_current_idt(void)
> > +{
> > +   if (atomic_long_read(&current_idt_descr_ptr))
> 
> Also, we should probably add here:
>               unsigned long new_idt = 
> atomic_long_read(&current_idt_descr_ptr);
> 
>               if (WARN_ON_ONCE(!validate_idt(new_idt))
>                       return;
>               load_idt((const struct desc_ptr *)new_idt);
> 
> > +           load_idt((const struct desc_ptr *)
> > +                    atomic_long_read(&current_idt_descr_ptr));
> > +   else
> > +           load_idt((const struct desc_ptr *)&idt_descr);
> > +}
> > +
> 
> Then have
> 
> bool validate_idt(unsigned long idt)
> {
>       switch(idt) {
>       case (unsigned long)&trace_idt_descr:
>       case (unsigned long)&idt_descr:
>               return 0;
>       }
>       return -1;
> }
> 
> This way we wont be opening up any easy root holes where if a process
> finds a way to modify some arbitrary kernel memory, we can prevent it
> from modifying the current_idt_descr_ptr and have a nice way to exploit
> the IDT. Sure, one can argue that if they can modify arbitrary kernel
> memory, we may already be lost, but lets not make it easier for them
> than need be.

I will introduce the validate_idt() as above in a next patch.

Thanks.

Seiji

> 
> -- Steve
> 

N�����r��y����b�X��ǧv�^�)޺{.n�+����{����zX����ܨ}���Ơz�&j:+v�������zZ+��+zf���h���~����i���z��w���?�����&�)ߢf��^jǫy�m��@A�a���
0��h���i