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>
CVE-2019-13233
(cherry picked from commit de9f869616dd95e95c00bdd6b0fcd3421e8a4323)
Signed-off-by: Benjamin M Romer <benjamin.romer@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
Acked-by: Connor Kuehl <connor.kuehl@canonical.com>
Signed-off-by: Khalid Elmously <khalid.elmously@canonical.com>

diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
index 9119d8e41f1ff59e2c8584a36f0f03d000bb1bbe..87dcba101e56268619453bf62c40a44dc29fbf6b 100644 (file)
--- a/arch/x86/lib/insn-eval.c
+++ b/arch/x86/lib/insn-eval.c
@@ -555,7 +555,8 @@ static int get_reg_offset_16(struct insn *insn, struct pt_regs *regs,
 }
 
 /**
- * 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.
@@ -563,18 +564,18 @@ static int get_reg_offset_16(struct insn *insn, struct pt_regs *regs,
  *
  * 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. */
@@ -582,12 +583,14 @@ static struct desc_struct *get_desc(unsigned short sel)
 
                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,9 +605,10 @@ static struct desc_struct *get_desc(unsigned short sel)
        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;
 }
 
 /**
@@ -626,7 +630,7 @@ static struct desc_struct *get_desc(unsigned short sel)
  */
 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);
@@ -664,11 +668,10 @@ unsigned long insn_get_seg_base(struct pt_regs *regs, int seg_reg_idx)
        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);
 }
 
 /**
@@ -690,7 +693,7 @@ 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)
 {
-       struct desc_struct *desc;
+       struct desc_struct desc;
        unsigned long limit;
        short sel;
 
@@ -704,8 +707,7 @@ static unsigned long get_seg_limit(struct pt_regs *regs, int seg_reg_idx)
        if (!sel)
                return 0;
 
-       desc = get_desc(sel);
-       if (!desc)
+       if (!get_desc(&desc, sel))
                return 0;
 
        /*
@@ -714,8 +716,8 @@ static unsigned long get_seg_limit(struct pt_regs *regs, int seg_reg_idx)
         * 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,7 +741,7 @@ 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;
+       struct desc_struct desc;
        short sel;
 
        if (v8086_mode(regs))
@@ -750,8 +752,7 @@ int insn_get_code_seg_params(struct pt_regs *regs)
        if (sel < 0)
                return sel;
 
-       desc = get_desc(sel);
-       if (!desc)
+       if (!get_desc(&desc, sel))
                return -EINVAL;
 
        /*
@@ -759,10 +760,10 @@ int insn_get_code_seg_params(struct pt_regs *regs)
         * 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.