lkml.org 
[lkml]   [2013]   [Jun]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v13 3/3] trace,x86: Add irq vector tracepoints
Date
> 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
>

\
 
 \ /
  Last update: 2013-06-04 18:41    [W:0.109 / U:0.672 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site