Messages in this thread | | | Date | Wed, 19 Apr 2017 12:26:36 +0200 | From | Borislav Petkov <> | Subject | Re: [v6 PATCH 07/21] x86/insn-eval: Add utility function to get segment descriptor |
| |
On Tue, Mar 07, 2017 at 04:32:40PM -0800, Ricardo Neri wrote: > The segment descriptor contains information that is relevant to how linear > address need to be computed. It contains the default size of addresses as > well as the base address of the segment. Thus, given a segment selector, > we ought look at segment descriptor to correctly calculate the linear > address. > > In protected mode, the segment selector might indicate a segment > descriptor from either the global descriptor table or a local descriptor > table. Both cases are considered in this function. > > This function is the initial implementation for subsequent functions that > will obtain the aforementioned attributes of the segment descriptor. > > Cc: Dave Hansen <dave.hansen@linux.intel.com> > Cc: Adam Buchbinder <adam.buchbinder@gmail.com> > Cc: Colin Ian King <colin.king@canonical.com> > Cc: Lorenzo Stoakes <lstoakes@gmail.com> > Cc: Qiaowei Ren <qiaowei.ren@intel.com> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com> > Cc: Masami Hiramatsu <mhiramat@kernel.org> > Cc: Adrian Hunter <adrian.hunter@intel.com> > Cc: Kees Cook <keescook@chromium.org> > Cc: Thomas Garnier <thgarnie@google.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Borislav Petkov <bp@suse.de> > Cc: Dmitry Vyukov <dvyukov@google.com> > Cc: Ravi V. Shankar <ravi.v.shankar@intel.com> > Cc: x86@kernel.org > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> > --- > arch/x86/lib/insn-eval.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 61 insertions(+) > > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c > index 8d45df8..8608adf 100644 > --- a/arch/x86/lib/insn-eval.c > +++ b/arch/x86/lib/insn-eval.c > @@ -5,9 +5,13 @@ > */ > #include <linux/kernel.h> > #include <linux/string.h> > +#include <asm/desc_defs.h> > +#include <asm/desc.h> > #include <asm/inat.h> > #include <asm/insn.h> > #include <asm/insn-eval.h> > +#include <asm/ldt.h> > +#include <linux/mmu_context.h> > #include <asm/vm86.h> > > enum reg_type { > @@ -294,6 +298,63 @@ static int get_reg_offset(struct insn *insn, struct pt_regs *regs, > } > > /** > + * get_desc() - Obtain address of segment descriptor > + * @seg: Segment selector
Maybe that should be
@sel
if it is a sel-ector. :)
And using "sel" makes more sense then when you look at:
desc_base = sel & ~(SEGMENT_RPL_MASK | SEGMENT_TI_MASK);
for example:
> + * @desc: Pointer to the selected segment descriptor > + * > + * Given a segment selector, obtain a memory pointer to the segment
s/memory //
> + * descriptor. Both global and local descriptor tables are supported. > + * desc will contain the address of the descriptor. > + * > + * Return: 0 if success, -EINVAL if failure
Why isn't this function returning the pointer or NULL on error? Maybe the later patches have an answer and I'll discover it if I continue reviewing :)
> + */ > +static int get_desc(unsigned short seg, struct desc_struct **desc) > +{ > + struct desc_ptr gdt_desc = {0, 0}; > + unsigned long desc_base; > + > + if (!desc) > + return -EINVAL; > + > + desc_base = seg & ~(SEGMENT_RPL_MASK | SEGMENT_TI_MASK);
That looks useless as you're doing it below again.
> + > +#ifdef CONFIG_MODIFY_LDT_SYSCALL > + if ((seg & SEGMENT_TI_MASK) == SEGMENT_LDT) { > + seg >>= 3; > + > + mutex_lock(¤t->active_mm->context.lock); > + if (unlikely(!current->active_mm->context.ldt ||
Is that really a fast path to complicate the if-test with an unlikely()? If not, you don't really need it.
> + seg >= current->active_mm->context.ldt->size)) {
ldt->size is the size of the descriptor table but you've shifted seg by 3. That selector index is shifted by 3 (to the left) to form an offset into the descriptor table because the entries there are 8 bytes.
So I *think* you wanna use the "useless" desc_base above... :)
> + *desc = NULL; > + mutex_unlock(¤t->active_mm->context.lock); > + return -EINVAL; > + } > + > + *desc = ¤t->active_mm->context.ldt->entries[seg];
... and seg here as it is an index into the table.
> + mutex_unlock(¤t->active_mm->context.lock); > + return 0; > + } > +#endif > + native_store_gdt(&gdt_desc); > + > + /* > + * Bits [15:3] of the segment selector contain the index. Such > + * index needs to be multiplied by 8.
... because <insert reason I typed in above>.
-- Regards/Gruss, Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --
| |