Re: [tip:x86/mpx] x86/insn-eval: Add utility function to get segment descriptor

From: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
To: Borislav Petkov <bp@suse.de>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	luto@kernel.org, adam.buchbinder@gmail.com, mst@redhat.com,
	mhiramat@kernel.org, dave.hansen@linux.intel.com,
	mingo@kernel.org, linux-kernel@vger.kernel.org,
	colin.king@canonical.com, jslaby@suse.cz, pbonzini@redhat.com,
	cmetcalf@mellanox.com, akpm@linux-foundation.org, vbabka@suse.cz,
	acme@redhat.com, brgerst@gmail.com, shuah@kernel.org,
	paul.gortmaker@windriver.com, lstoakes@gmail.com, hpa@zytor.com,
	thgarnie@google.com, keescook@chromium.org,
	adrian.hunter@intel.com, ray.huang@amd.com, dvyukov@google.com,
	ravi.v.shankar@intel.com, slaoub@gmail.com, corbet@lwn.net,
	linux-tip-commits@vger.kernel.org
Subject: Re: [tip:x86/mpx] x86/insn-eval: Add utility function to get segment descriptor
Date: Wed, 6 Dec 2017 23:23:59 -0800	[thread overview]
Message-ID: <20171207072359.GA19428@voyager> (raw)
In-Reply-To: <20171205212933.amf4pkpzgrjpdssw@pd.tnic>

On Tue, Dec 05, 2017 at 10:29:33PM +0100, Borislav Petkov wrote:
> On Tue, Dec 05, 2017 at 07:38:45PM +0100, Peter Zijlstra wrote:
> > Sorry what? So either this code is broken because it has IRQs enabled,
> > or its broken because its trying to acquire a mutex with IRQs disabled.
> > Which is it?
> 
> Well, lemme try to sum up what Peter, Thomas and I discussed on IRC:
> 
> The problem is that there's no guarantee userspace won't change the LDT
> from under us while the UMIP code runs in the insn decoder.

Yes, I see the problem now.
> 
> So, we need a way to be able to query the desc fields the insn decoder
> needs *and* when the LDT changes through the syscall, to detect that
> case and handle it gracefully in the decoder.
> 
> So Thomas' idea is to keep a mm->context.ldt_seq sequence number which
> gets incremented (and wraps around) everytime a LDT changes.
> 
> That sequence number, i.e., cookie, gets handed down into the decoder
> and it uses it during desc lookup. If the sequence number changes, the
> decoder and the UMIP code must abort the emulation.

In UMIP emulation we can potentially access the LDT twice. Once when
determining the base address of the code segment and again when determining
the base address and limit of the segment in which the result of the
emulation is written. I guess that mm->context.ldt_seq needs to not change
not only while decoding a particular linear address but across these two
linear address decodings.
> 
> The lookup code needs to do that with IRQs disabled, of course, to
> protect itself from IPIs which could change the LDT.
> 
> I *think* this is the gist of what we talked about, tglx, please correct
> me if I missed something.
> 
> So, Ricardo, please take a look at fixing that as otherwise the UMIP
> code would choke and possibly rely on wrong data. If there are any
> questions, don't hesitate to ask.

Sure, I will look into implementing this idea and post patches for it.

Thanks and BR,
Ricardo