[[b,c,d,any] ][PATCH 1/1] x86/insn-eval: Fix use-after-free access to LDT entry
diff mbox series

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
Related show

Commit Message

Benjamin M Romer July 16, 2019, 5:08 p.m. UTC
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>
---
 arch/x86/lib/insn-eval.c | 47 ++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

Comments

Connor Kuehl July 17, 2019, 5:14 p.m. UTC | #1
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(&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.
>
Stefan Bader July 18, 2019, 9:51 a.m. UTC | #2
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(&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.
>

Patch
diff mbox series

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(&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.