Message-ID: <1370303581.26799.109.camel@gandalf.local.home>
Subject: Re: [PATCH v13 3/3] trace,x86: Add irq vector tracepoints
From: Steven Rostedt <rostedt@goodmis.org>
To: Seiji Aguchi <seiji.aguchi@hds.com>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org, hpa@zytor.com,
        tglx@linutronix.de, mingo@elte.hu, bp@alien8.de,
        linux-edac@vger.kernel.org, tony.luck@intel.com,
        dle-develop@lists.sourceforge.net, tomoki.sekiyama@hds.com
Date: Mon, 03 Jun 2013 19:53:01 -0400
In-Reply-To: <51ACEEB4.1010302@hds.com>
References: <51ACEDBB.6040706@hds.com> <51ACEEB4.1010302@hds.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1742
Lines: 59

On Mon, 2013-06-03 at 15:29 -0400, Seiji Aguchi wrote:

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

/*
 * The current_idt_descr_ptr can only be set out of interrupt context
 * to avoid races. 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.
 */
>  
> +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.

-- Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/