| Message ID | 20190716170824.31242-2-benjamin.romer@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [[b,c,d,any] ][PATCH 1/1] x86/insn-eval: Fix use-after-free access to LDT entry | expand |
On 7/16/19 10:08 AM, Benjamin M Romer wrote: > From: Jann Horn <jannh@google.com> > > 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> > > (cherry picked from commit de9f869616dd95e95c00bdd6b0fcd3421e8a4323) > CVE-2019-13233 > Signed-off-by: Benjamin M Romer <benjamin.romer@canonical.com> Acked-by: Connor Kuehl <connor.kuehl@canonical.com> > --- > arch/x86/lib/insn-eval.c | 47 ++++++++++++++++++++-------------------- > 1 file changed, 24 insertions(+), 23 deletions(-) > > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c > index 9119d8e41f1f..87dcba101e56 100644 > --- 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(¤t->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(¤t->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. >
On 16.07.19 19:08, Benjamin M Romer wrote: > From: Jann Horn <jannh@google.com> > > 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> > > (cherry picked from commit de9f869616dd95e95c00bdd6b0fcd3421e8a4323) > CVE-2019-13233 > Signed-off-by: Benjamin M Romer <benjamin.romer@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- Usually we put the CVE number or BugLink before the cherry pick and sob section (would propose to do that when applying). NACK for Cosmic as that is EOL. -Stefan > arch/x86/lib/insn-eval.c | 47 ++++++++++++++++++++-------------------- > 1 file changed, 24 insertions(+), 23 deletions(-) > > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c > index 9119d8e41f1f..87dcba101e56 100644 > --- 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(¤t->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(¤t->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. >
diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c index 9119d8e41f1f..87dcba101e56 100644 --- 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(¤t->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(¤t->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.