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