CVE-2019-13233: Concurrent Execution using Shared Resource with Improper Synchronization ('Race Condition') (CWE-362)
value: In arch/x86/lib/insn-eval.c in the Linux kernel before 5.1.9, there is a use-after-free for access to an LDT entry because of a race condition between modify_ldt() and a #BR exception for an MPX bounds violation.
CWE description: The product contains a code sequence that can run concurrently with other code, and the code sequence requires temporary, exclusive access to a shared resource, but a timing window exists in which the shared resource can be modified by another code sequence that is operating concurrently.
CVSS v3 base score: 7
Published: 2019-07-04T13:15Z
Repository: https://github.com/torvalds/linux
Language: C
Files changed: arch/x86/lib/insn-eval.c
Commit date: 2019-06-07 11:11:06 -0700
Commit message: x86/insn-eval: Fix use-after-free access to LDT entry

get_desc() computes a pointer into the LDT while holding a lock that
protects the LDT from being freed, but then drops the lock and returns the
(now potentially dangling) pointer to its caller.

Fix it by giving the caller a copy of the LDT entry instead.

Fixes: 670f928ba09b ("x86/insn-eval: Add utility function to get segment descriptor")
Cc: stable@vger.kernel.org
Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

Diff: diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
index cf00ab6c6621..306c3a0902ba 100644
--- a/arch/x86/lib/insn-eval.c
+++ b/arch/x86/lib/insn-eval.c
@@ -555,43 +555,46 @@ static int get_reg_offset_16(struct insn *insn, struct pt_regs *regs,
 
 	return 0;
 }
 
 /**
- * get_desc() - Obtain pointer to a segment descriptor
+ * get_desc() - Obtain contents of a segment descriptor
+ * @out:	Segment descriptor contents on success
  * @sel:	Segment selector
  *
  * Given a segment selector, obtain a pointer to the segment descriptor.
  * Both global and local descriptor tables are supported.
  *
  * Returns:
  *
- * Pointer to segment descriptor on success.
+ * True on success, false on failure.
  *
  * NULL on error.
  */
-static struct desc_struct *get_desc(unsigned short sel)
+static bool get_desc(struct desc_struct *out, 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;
+		bool success = false;
 		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];
+		if (ldt && sel < ldt->nr_entries) {
+			*out = ldt->entries[sel];
+			success = true;
+		}
 
 		mutex_unlock(&current->active_mm->context.lock);
 
-		return desc;
+		return success;
 	}
 #endif
 	native_store_gdt(&gdt_desc);
 
 	/*
@@ -602,13 +605,14 @@ static struct desc_struct *get_desc(unsigned short sel)
 	 * All that remains is to clear bits [2:0].
 	 */
 	desc_base = sel & ~(SEGMENT_RPL_MASK | SEGMENT_TI_MASK);
 
 	if (desc_base > gdt_desc.size)
-		return NULL;
+		return false;
 
-	return (struct desc_struct *)(gdt_desc.address + desc_base);
+	*out = *(struct desc_struct *)(gdt_desc.address + desc_base);
+	return true;
 }
 
 /**
  * insn_get_seg_base() - Obtain base address of segment descriptor.
  * @regs:		Register values as seen when entering kernel mode
@@ -626,11 +630,11 @@ static struct desc_struct *get_desc(unsigned short sel)
  *
  * -1L in case of error.
  */
 unsigned long insn_get_seg_base(struct pt_regs *regs, int seg_reg_idx)
 {
-	struct desc_struct *desc;
+	struct desc_struct desc;
 	short sel;
 
 	sel = get_segment_selector(regs, seg_reg_idx);
 	if (sel < 0)
 		return -1L;
@@ -664,15 +668,14 @@ unsigned long insn_get_seg_base(struct pt_regs *regs, int seg_reg_idx)
 
 	/* In protected mode the segment selector cannot be null. */
 	if (!sel)
 		return -1L;
 
-	desc = get_desc(sel);
-	if (!desc)
+	if (!get_desc(&desc, sel))
 		return -1L;
 
-	return get_desc_base(desc);
+	return get_desc_base(&desc);
 }
 
 /**
  * get_seg_limit() - Obtain the limit of a segment descriptor
  * @regs:		Register values as seen when entering kernel mode
@@ -690,11 +693,11 @@ unsigned long insn_get_seg_base(struct pt_regs *regs, int seg_reg_idx)
  *
  * Zero is returned on error.
  */
 static unsigned long get_seg_limit(struct pt_regs *regs, int seg_reg_idx)
 {
-	struct desc_struct *desc;
+	struct desc_struct desc;
 	unsigned long limit;
 	short sel;
 
 	sel = get_segment_selector(regs, seg_reg_idx);
 	if (sel < 0)
@@ -704,22 +707,21 @@ static unsigned long get_seg_limit(struct pt_regs *regs, int seg_reg_idx)
 		return -1L;
 
 	if (!sel)
 		return 0;
 
-	desc = get_desc(sel);
-	if (!desc)
+	if (!get_desc(&desc, sel))
 		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 = get_desc_limit(&desc);
+	if (desc.g)
 		limit = (limit << 12) + 0xfff;
 
 	return limit;
 }
 
@@ -739,34 +741,33 @@ static unsigned long get_seg_limit(struct pt_regs *regs, int seg_reg_idx)
  *
  * -EINVAL on error.
  */
 int insn_get_code_seg_params(struct pt_regs *regs)
 {
-	struct desc_struct *desc;
+	struct desc_struct desc;
 	short sel;
 
 	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;
 
-	desc = get_desc(sel);
-	if (!desc)
+	if (!get_desc(&desc, sel))
 		return -EINVAL;
 
 	/*
 	 * The most significant byte of the Type field of the segment descriptor
 	 * determines whether a segment contains data or code. If this is a data
 	 * segment, return error.
 	 */
-	if (!(desc->type & BIT(3)))
+	if (!(desc.type & BIT(3)))
 		return -EINVAL;
 
-	switch ((desc->l << 1) | desc->d) {
+	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);

Vulnerable code:  * get_desc() - Obtain pointer to a segment descriptor
 * Pointer to segment descriptor on success.
static struct desc_struct *get_desc(unsigned short sel)
		struct desc_struct *desc = NULL;
		if (ldt && sel < ldt->nr_entries)
			desc = &ldt->entries[sel];
		return desc;
		return NULL;
	return (struct desc_struct *)(gdt_desc.address + desc_base);
	struct desc_struct *desc;
	desc = get_desc(sel);
	if (!desc)
	return get_desc_base(desc);
	struct desc_struct *desc;
	desc = get_desc(sel);
	if (!desc)
	limit = get_desc_limit(desc);
	if (desc->g)
	struct desc_struct *desc;
	desc = get_desc(sel);
	if (!desc)
	if (!(desc->type & BIT(3)))
	switch ((desc->l << 1) | desc->d) {
Fixed code:  * get_desc() - Obtain contents of a segment descriptor
 * @out:	Segment descriptor contents on success
 * True on success, false on failure.
static bool get_desc(struct desc_struct *out, unsigned short sel)
		bool success = false;
		if (ldt && sel < ldt->nr_entries) {
			*out = ldt->entries[sel];
			success = true;
		}
		return success;
		return false;
	*out = *(struct desc_struct *)(gdt_desc.address + desc_base);
	return true;
	struct desc_struct desc;
	if (!get_desc(&desc, sel))
	return get_desc_base(&desc);
	struct desc_struct desc;
	if (!get_desc(&desc, sel))
	limit = get_desc_limit(&desc);
	if (desc.g)
	struct desc_struct desc;
	if (!get_desc(&desc, sel))
	if (!(desc.type & BIT(3)))
	switch ((desc.l << 1) | desc.d) {