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

[Peter Zijlstra <peterz@infradead.org>]

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(&current->active_mm->context.lock);
> +		ldt = current->active_mm->context.ldt;
> +		if (ldt && sel < ldt->nr_entries)
> +			desc = &ldt->entries[sel];
> +
> +		mutex_unlock(&current->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(&current->active_mm->context.lock);
 		ldt = current->active_mm->context.ldt;
 		if (ldt && sel < ldt->nr_entries)
 			desc = &ldt->entries_va[sel];
 
-		mutex_unlock(&current->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;
 }
 
 /**