On Wed, Nov 01, 2017 at 02:00:28PM -0700, tip-bot for Ricardo Neri wrote:
> +static struct desc_struct *get_desc(unsigned short sel)
> +{
> + struct desc_ptr gdt_desc = {0, 0};
> + unsigned long desc_base;
> +
> +#ifdef CONFIG_MODIFY_LDT_SYSCALL
> + if ((sel & SEGMENT_TI_MASK) == SEGMENT_LDT) {
> + struct desc_struct *desc = NULL;
> + struct ldt_struct *ldt;
> +
> + /* Bits [15:3] contain the index of the desired entry. */
> + sel >>= 3;
> +
> + mutex_lock(¤t->active_mm->context.lock);
> + ldt = current->active_mm->context.ldt;
> + if (ldt && sel < ldt->nr_entries)
> + desc = &ldt->entries[sel];
> +
> + mutex_unlock(¤t->active_mm->context.lock);
> +
> + return desc;
> + }
> +#endif
This is broken right? You unlock and then return @desc, which afaict can
at that point get freed by free_ldt_struct().
Something like the below ought to cure; although its not entirely
pretty either.
---
diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
index e664058c4491..c234ef2b4430 100644
--- a/arch/x86/lib/insn-eval.c
+++ b/arch/x86/lib/insn-eval.c
@@ -572,6 +572,11 @@ static struct desc_struct *get_desc(unsigned short sel)
struct desc_ptr gdt_desc = {0, 0};
unsigned long desc_base;
+ /*
+ * Relies on IRQs being disabled to serialize against the LDT.
+ */
+ lockdep_assert_irqs_disabled();
+
#ifdef CONFIG_MODIFY_LDT_SYSCALL
if ((sel & SEGMENT_TI_MASK) == SEGMENT_LDT) {
struct desc_struct *desc = NULL;
@@ -580,13 +585,10 @@ static struct desc_struct *get_desc(unsigned short sel)
/* Bits [15:3] contain the index of the desired entry. */
sel >>= 3;
- mutex_lock(¤t->active_mm->context.lock);
ldt = current->active_mm->context.ldt;
if (ldt && sel < ldt->nr_entries)
desc = &ldt->entries_va[sel];
- mutex_unlock(¤t->active_mm->context.lock);
-
return desc;
}
#endif
@@ -626,6 +628,7 @@ static struct desc_struct *get_desc(unsigned short sel)
*/
unsigned long insn_get_seg_base(struct pt_regs *regs, int seg_reg_idx)
{
+ unsigned long base, flags;
struct desc_struct *desc;
short sel;
@@ -664,11 +667,15 @@ unsigned long insn_get_seg_base(struct pt_regs *regs, int
seg_reg_idx)
if (!sel)
return -1L;
+ base = -1;
+
+ local_irq_save(flags);
desc = get_desc(sel);
- if (!desc)
- return -1L;
+ if (desc)
+ base = get_desc_base(desc);
+ local_irq_restore(flags);
- return get_desc_base(desc);
+ return base;
}
/**
@@ -690,8 +697,8 @@ unsigned long insn_get_seg_base(struct pt_regs *regs, int
seg_reg_idx)
*/
static unsigned long get_seg_limit(struct pt_regs *regs, int seg_reg_idx)
{
+ unsigned long flags, limit = 0;
struct desc_struct *desc;
- unsigned long limit;
short sel;
sel = get_segment_selector(regs, seg_reg_idx);
@@ -704,19 +711,20 @@ static unsigned long get_seg_limit(struct pt_regs *regs,
int seg_reg_idx)
if (!sel)
return 0;
+ local_irq_save(flags);
desc = get_desc(sel);
- if (!desc)
- return 0;
-
- /*
- * If the granularity bit is set, the limit is given in multiples
- * of 4096. This also means that the 12 least significant bits are
- * not tested when checking the segment limits. In practice,
- * this means that the segment ends in (limit << 12) + 0xfff.
- */
- limit = get_desc_limit(desc);
- if (desc->g)
- limit = (limit << 12) + 0xfff;
+ if (desc) {
+ /*
+ * If the granularity bit is set, the limit is given in
multiples
+ * of 4096. This also means that the 12 least significant bits
are
+ * not tested when checking the segment limits. In practice,
+ * this means that the segment ends in (limit << 12) + 0xfff.
+ */
+ limit = get_desc_limit(desc);
+ if (desc->g)
+ limit = (limit << 12) + 0xfff;
+ }
+ local_irq_restore(flags);
return limit;
}
@@ -740,19 +748,23 @@ static unsigned long get_seg_limit(struct pt_regs *regs,
int seg_reg_idx)
int insn_get_code_seg_params(struct pt_regs *regs)
{
struct desc_struct *desc;
+ unsigned long flags;
+ int ret = -EINVAL;
short sel;
- if (v8086_mode(regs))
+ if (v8086_mode(regs)) {
/* Address and operand size are both 16-bit. */
return INSN_CODE_SEG_PARAMS(2, 2);
+ }
sel = get_segment_selector(regs, INAT_SEG_REG_CS);
if (sel < 0)
return sel;
+ local_irq_save(flags);
desc = get_desc(sel);
if (!desc)
- return -EINVAL;
+ goto out;
/*
* The most significant byte of the Type field of the segment descriptor
@@ -760,29 +772,37 @@ int insn_get_code_seg_params(struct pt_regs *regs)
* segment, return error.
*/
if (!(desc->type & BIT(3)))
- return -EINVAL;
+ goto out;
switch ((desc->l << 1) | desc->d) {
case 0: /*
* Legacy mode. CS.L=0, CS.D=0. Address and operand size are
* both 16-bit.
*/
- return INSN_CODE_SEG_PARAMS(2, 2);
+ ret = INSN_CODE_SEG_PARAMS(2, 2);
+ break;
case 1: /*
* Legacy mode. CS.L=0, CS.D=1. Address and operand size are
* both 32-bit.
*/
- return INSN_CODE_SEG_PARAMS(4, 4);
+ ret = INSN_CODE_SEG_PARAMS(4, 4);
+ break;
case 2: /*
* IA-32e 64-bit mode. CS.L=1, CS.D=0. Address size is 64-bit;
* operand size is 32-bit.
*/
- return INSN_CODE_SEG_PARAMS(4, 8);
+ ret = INSN_CODE_SEG_PARAMS(4, 8);
+ break;
+
case 3: /* Invalid setting. CS.L=1, CS.D=1 */
/* fall through */
default:
- return -EINVAL;
+ break;
}
+out:
+ local_irq_restore(flags);
+
+ return ret;
}
/**
