Messages in this thread | | | From | Seiji Aguchi <> | Subject | RE: [PATCH v13 3/3] trace,x86: Add irq vector tracepoints | Date | Tue, 4 Jun 2013 15:50:17 +0000 |
| |
> 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(¤t_idt_descr_ptr)) > > Also, we should probably add here: > unsigned long new_idt = atomic_long_read(¤t_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(¤t_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 >
| |