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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 4 Jun 2013 15:50:17 +0000
From:	Seiji Aguchi <seiji.aguchi@....com>
To:	Steven Rostedt <rostedt@...dmis.org>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"x86@...nel.org" <x86@...nel.org>, "hpa@...or.com" <hpa@...or.com>,
	"tglx@...utronix.de" <tglx@...utronix.de>,
	"mingo@...e.hu" <mingo@...e.hu>, "bp@...en8.de" <bp@...en8.de>,
	"linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>,
	"tony.luck@...el.com" <tony.luck@...el.com>,
	"dle-develop@...ts.sourceforge.net" 
	<dle-develop@...ts.sourceforge.net>,
	Tomoki Sekiyama <tomoki.sekiyama@....com>
Subject: RE: [PATCH v13 3/3] trace,x86: Add irq vector tracepoints

> 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
>