[v1] misc: sgi-gru: fix use-after-free error in gru_set_context_option, gru_fault and gru_handle_user_call_os

[PATCH] misc: sgi-gru: fix use-after-free error in gru_set_context_option, gru_fault and gru_handle_user_call_os

Posted by Zheng Wang 3 years, 6 months ago

In grufile.c, gru_file_unlocked_ioctl function can be called by user.

If the req is GRU_SET_CONTEXT_OPTION, it will call gru_set_context_option.

In gru_set_context_option, as req can be controlled by user,

We can reach gru_check_context_placement function call.

In gru_check_context_placement function, if the error path was steped,

say gru_check_chiplet_assignment return 0,

Then it will fall into gru_unload_context function.

And it will finnaly call kfree gts in gts_drop function.

Then gru_unlock_gts will be called in gru_set_context_option function.

This is a typical Use after free.

The same problem exists in gru_handle_user_call_os and gru_fault.

Fix it by introduce the return value to see if gts is in good case or not.

Free the gts in caller when gru_check_chiplet_assignment check failed.

Reported-by: Zheng Wang <hackerzheng666@gmail.com> Zhuorao Yang <alex000young@gmail.com>

Signed-off-by: Zheng Wang <zyytlz.wz@163.com>
---
 drivers/misc/sgi-gru/grufault.c  | 14 ++++++++++++--
 drivers/misc/sgi-gru/grumain.c   | 17 +++++++++++++----
 drivers/misc/sgi-gru/grutables.h |  2 +-
 3 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/misc/sgi-gru/grufault.c b/drivers/misc/sgi-gru/grufault.c
index d7ef61e602ed..2b5b049fbd38 100644
--- a/drivers/misc/sgi-gru/grufault.c
+++ b/drivers/misc/sgi-gru/grufault.c
@@ -656,7 +656,9 @@ int gru_handle_user_call_os(unsigned long cb)
 	if (ucbnum >= gts->ts_cbr_au_count * GRU_CBR_AU_SIZE)
 		goto exit;
 
-	gru_check_context_placement(gts);
+	ret = gru_check_context_placement(gts);
+	if (ret)
+		goto err;
 
 	/*
 	 * CCH may contain stale data if ts_force_cch_reload is set.
@@ -677,6 +679,10 @@ int gru_handle_user_call_os(unsigned long cb)
 exit:
 	gru_unlock_gts(gts);
 	return ret;
+err:
+	gru_unlock_gts(gts);
+	gru_unload_context(gts, 1);
+	return -EINVAL;
 }
 
 /*
@@ -874,7 +880,7 @@ int gru_set_context_option(unsigned long arg)
 		} else {
 			gts->ts_user_blade_id = req.val1;
 			gts->ts_user_chiplet_id = req.val0;
-			gru_check_context_placement(gts);
+			ret = gru_check_context_placement(gts);
 		}
 		break;
 	case sco_gseg_owner:
@@ -889,6 +895,10 @@ int gru_set_context_option(unsigned long arg)
 		ret = -EINVAL;
 	}
 	gru_unlock_gts(gts);
+	if (ret) {
+		gru_unload_context(gts, 1);
+		ret = -EINVAL;
+	}
 
 	return ret;
 }
diff --git a/drivers/misc/sgi-gru/grumain.c b/drivers/misc/sgi-gru/grumain.c
index 9afda47efbf2..79903cf7e706 100644
--- a/drivers/misc/sgi-gru/grumain.c
+++ b/drivers/misc/sgi-gru/grumain.c
@@ -716,9 +716,10 @@ static int gru_check_chiplet_assignment(struct gru_state *gru,
  * chiplet. Misassignment can occur if the process migrates to a different
  * blade or if the user changes the selected blade/chiplet.
  */
-void gru_check_context_placement(struct gru_thread_state *gts)
+int gru_check_context_placement(struct gru_thread_state *gts)
 {
 	struct gru_state *gru;
+	int ret = 0;
 
 	/*
 	 * If the current task is the context owner, verify that the
@@ -727,14 +728,16 @@ void gru_check_context_placement(struct gru_thread_state *gts)
 	 */
 	gru = gts->ts_gru;
 	if (!gru || gts->ts_tgid_owner != current->tgid)
-		return;
+		return ret;
 
 	if (!gru_check_chiplet_assignment(gru, gts)) {
 		STAT(check_context_unload);
-		gru_unload_context(gts, 1);
+		ret = -EINVAL;
 	} else if (gru_retarget_intr(gts)) {
 		STAT(check_context_retarget_intr);
 	}
+
+	return ret;
 }
 
 
@@ -919,6 +922,7 @@ vm_fault_t gru_fault(struct vm_fault *vmf)
 	struct gru_thread_state *gts;
 	unsigned long paddr, vaddr;
 	unsigned long expires;
+	int ret;
 
 	vaddr = vmf->address;
 	gru_dbg(grudev, "vma %p, vaddr 0x%lx (0x%lx)\n",
@@ -934,7 +938,12 @@ vm_fault_t gru_fault(struct vm_fault *vmf)
 	mutex_lock(&gts->ts_ctxlock);
 	preempt_disable();
 
-	gru_check_context_placement(gts);
+	ret = gru_check_context_placement(gts);
+	if (ret) {
+		mutex_unlock(&gts->ts_ctxlock);
+		gru_unload_context(gts, 1);
+		return ret;
+	}
 
 	if (!gts->ts_gru) {
 		STAT(load_user_context);
diff --git a/drivers/misc/sgi-gru/grutables.h b/drivers/misc/sgi-gru/grutables.h
index 5efc869fe59a..f4a5a787685f 100644
--- a/drivers/misc/sgi-gru/grutables.h
+++ b/drivers/misc/sgi-gru/grutables.h
@@ -632,7 +632,7 @@ extern int gru_user_flush_tlb(unsigned long arg);
 extern int gru_user_unload_context(unsigned long arg);
 extern int gru_get_exception_detail(unsigned long arg);
 extern int gru_set_context_option(unsigned long address);
-extern void gru_check_context_placement(struct gru_thread_state *gts);
+extern int gru_check_context_placement(struct gru_thread_state *gts);
 extern int gru_cpu_fault_map_id(void);
 extern struct vm_area_struct *gru_find_vma(unsigned long vaddr);
 extern void gru_flush_all_tlb(struct gru_state *gru);
-- 
2.25.1

Re: [PATCH] misc: sgi-gru: fix use-after-free error in gru_set_context_option, gru_fault and gru_handle_user_call_os

Posted by Greg KH 3 years, 6 months ago

On Mon, Sep 19, 2022 at 10:32:05PM +0800, Zheng Wang wrote:
> In grufile.c, gru_file_unlocked_ioctl function can be called by user.
> 
> If the req is GRU_SET_CONTEXT_OPTION, it will call gru_set_context_option.
> 
> In gru_set_context_option, as req can be controlled by user,
> 
> We can reach gru_check_context_placement function call.
> 
> In gru_check_context_placement function, if the error path was steped,
> 
> say gru_check_chiplet_assignment return 0,
> 
> Then it will fall into gru_unload_context function.
> 
> And it will finnaly call kfree gts in gts_drop function.
> 
> Then gru_unlock_gts will be called in gru_set_context_option function.
> 
> This is a typical Use after free.
> 
> The same problem exists in gru_handle_user_call_os and gru_fault.
> 
> Fix it by introduce the return value to see if gts is in good case or not.
> 
> Free the gts in caller when gru_check_chiplet_assignment check failed.

Your text formatting is a bit odd, don't you think?

> 
> Reported-by: Zheng Wang <hackerzheng666@gmail.com> Zhuorao Yang <alex000young@gmail.com>

Why twice?

Should be two different reported-by lines, right?

Otherwise looks good, can you fix that up and resend?

thanks,

greg k-h

Re: [PATCH] misc: sgi-gru: fix use-after-free error in gru_set_context_option, gru_fault and gru_handle_user_call_os

Posted by Zheng Hacker 3 years, 6 months ago

Hi Greg,

Very happy to receive your email. I was outgoing in the weekend and
sorry for my late reply. I'll fix the patch and resend it to you.

Best regards,
Zheng Wang

Greg KH <gregkh@linuxfoundation.org> 于2022年9月24日周六 20:54写道：
>
> On Mon, Sep 19, 2022 at 10:32:05PM +0800, Zheng Wang wrote:
> > In grufile.c, gru_file_unlocked_ioctl function can be called by user.
> >
> > If the req is GRU_SET_CONTEXT_OPTION, it will call gru_set_context_option.
> >
> > In gru_set_context_option, as req can be controlled by user,
> >
> > We can reach gru_check_context_placement function call.
> >
> > In gru_check_context_placement function, if the error path was steped,
> >
> > say gru_check_chiplet_assignment return 0,
> >
> > Then it will fall into gru_unload_context function.
> >
> > And it will finnaly call kfree gts in gts_drop function.
> >
> > Then gru_unlock_gts will be called in gru_set_context_option function.
> >
> > This is a typical Use after free.
> >
> > The same problem exists in gru_handle_user_call_os and gru_fault.
> >
> > Fix it by introduce the return value to see if gts is in good case or not.
> >
> > Free the gts in caller when gru_check_chiplet_assignment check failed.
>
> Your text formatting is a bit odd, don't you think?
>
> >
> > Reported-by: Zheng Wang <hackerzheng666@gmail.com> Zhuorao Yang <alex000young@gmail.com>
>
> Why twice?
>
> Should be two different reported-by lines, right?
>
> Otherwise looks good, can you fix that up and resend?
>
> thanks,
>
> greg k-h

Re: [PATCH] misc: sgi-gru: fix use-after-free error in gru_set_context_option, gru_fault and gru_handle_user_call_os

Posted by Zheng Hacker 3 years, 6 months ago

Hi,

I've sent the patch with anothe account.
Could you please try this one?

Regards,
Zheng Wang

Zheng Wang <zyytlz.wz@163.com> 于2022年9月19日周一 22:32写道：
>
> In grufile.c, gru_file_unlocked_ioctl function can be called by user.
>
> If the req is GRU_SET_CONTEXT_OPTION, it will call gru_set_context_option.
>
> In gru_set_context_option, as req can be controlled by user,
>
> We can reach gru_check_context_placement function call.
>
> In gru_check_context_placement function, if the error path was steped,
>
> say gru_check_chiplet_assignment return 0,
>
> Then it will fall into gru_unload_context function.
>
> And it will finnaly call kfree gts in gts_drop function.
>
> Then gru_unlock_gts will be called in gru_set_context_option function.
>
> This is a typical Use after free.
>
> The same problem exists in gru_handle_user_call_os and gru_fault.
>
> Fix it by introduce the return value to see if gts is in good case or not.
>
> Free the gts in caller when gru_check_chiplet_assignment check failed.
>
> Reported-by: Zheng Wang <hackerzheng666@gmail.com> Zhuorao Yang <alex000young@gmail.com>
>
> Signed-off-by: Zheng Wang <zyytlz.wz@163.com>
> ---
>  drivers/misc/sgi-gru/grufault.c  | 14 ++++++++++++--
>  drivers/misc/sgi-gru/grumain.c   | 17 +++++++++++++----
>  drivers/misc/sgi-gru/grutables.h |  2 +-
>  3 files changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/misc/sgi-gru/grufault.c b/drivers/misc/sgi-gru/grufault.c
> index d7ef61e602ed..2b5b049fbd38 100644
> --- a/drivers/misc/sgi-gru/grufault.c
> +++ b/drivers/misc/sgi-gru/grufault.c
> @@ -656,7 +656,9 @@ int gru_handle_user_call_os(unsigned long cb)
>         if (ucbnum >= gts->ts_cbr_au_count * GRU_CBR_AU_SIZE)
>                 goto exit;
>
> -       gru_check_context_placement(gts);
> +       ret = gru_check_context_placement(gts);
> +       if (ret)
> +               goto err;
>
>         /*
>          * CCH may contain stale data if ts_force_cch_reload is set.
> @@ -677,6 +679,10 @@ int gru_handle_user_call_os(unsigned long cb)
>  exit:
>         gru_unlock_gts(gts);
>         return ret;
> +err:
> +       gru_unlock_gts(gts);
> +       gru_unload_context(gts, 1);
> +       return -EINVAL;
>  }
>
>  /*
> @@ -874,7 +880,7 @@ int gru_set_context_option(unsigned long arg)
>                 } else {
>                         gts->ts_user_blade_id = req.val1;
>                         gts->ts_user_chiplet_id = req.val0;
> -                       gru_check_context_placement(gts);
> +                       ret = gru_check_context_placement(gts);
>                 }
>                 break;
>         case sco_gseg_owner:
> @@ -889,6 +895,10 @@ int gru_set_context_option(unsigned long arg)
>                 ret = -EINVAL;
>         }
>         gru_unlock_gts(gts);
> +       if (ret) {
> +               gru_unload_context(gts, 1);
> +               ret = -EINVAL;
> +       }
>
>         return ret;
>  }
> diff --git a/drivers/misc/sgi-gru/grumain.c b/drivers/misc/sgi-gru/grumain.c
> index 9afda47efbf2..79903cf7e706 100644
> --- a/drivers/misc/sgi-gru/grumain.c
> +++ b/drivers/misc/sgi-gru/grumain.c
> @@ -716,9 +716,10 @@ static int gru_check_chiplet_assignment(struct gru_state *gru,
>   * chiplet. Misassignment can occur if the process migrates to a different
>   * blade or if the user changes the selected blade/chiplet.
>   */
> -void gru_check_context_placement(struct gru_thread_state *gts)
> +int gru_check_context_placement(struct gru_thread_state *gts)
>  {
>         struct gru_state *gru;
> +       int ret = 0;
>
>         /*
>          * If the current task is the context owner, verify that the
> @@ -727,14 +728,16 @@ void gru_check_context_placement(struct gru_thread_state *gts)
>          */
>         gru = gts->ts_gru;
>         if (!gru || gts->ts_tgid_owner != current->tgid)
> -               return;
> +               return ret;
>
>         if (!gru_check_chiplet_assignment(gru, gts)) {
>                 STAT(check_context_unload);
> -               gru_unload_context(gts, 1);
> +               ret = -EINVAL;
>         } else if (gru_retarget_intr(gts)) {
>                 STAT(check_context_retarget_intr);
>         }
> +
> +       return ret;
>  }
>
>
> @@ -919,6 +922,7 @@ vm_fault_t gru_fault(struct vm_fault *vmf)
>         struct gru_thread_state *gts;
>         unsigned long paddr, vaddr;
>         unsigned long expires;
> +       int ret;
>
>         vaddr = vmf->address;
>         gru_dbg(grudev, "vma %p, vaddr 0x%lx (0x%lx)\n",
> @@ -934,7 +938,12 @@ vm_fault_t gru_fault(struct vm_fault *vmf)
>         mutex_lock(&gts->ts_ctxlock);
>         preempt_disable();
>
> -       gru_check_context_placement(gts);
> +       ret = gru_check_context_placement(gts);
> +       if (ret) {
> +               mutex_unlock(&gts->ts_ctxlock);
> +               gru_unload_context(gts, 1);
> +               return ret;
> +       }
>
>         if (!gts->ts_gru) {
>                 STAT(load_user_context);
> diff --git a/drivers/misc/sgi-gru/grutables.h b/drivers/misc/sgi-gru/grutables.h
> index 5efc869fe59a..f4a5a787685f 100644
> --- a/drivers/misc/sgi-gru/grutables.h
> +++ b/drivers/misc/sgi-gru/grutables.h
> @@ -632,7 +632,7 @@ extern int gru_user_flush_tlb(unsigned long arg);
>  extern int gru_user_unload_context(unsigned long arg);
>  extern int gru_get_exception_detail(unsigned long arg);
>  extern int gru_set_context_option(unsigned long address);
> -extern void gru_check_context_placement(struct gru_thread_state *gts);
> +extern int gru_check_context_placement(struct gru_thread_state *gts);
>  extern int gru_cpu_fault_map_id(void);
>  extern struct vm_area_struct *gru_find_vma(unsigned long vaddr);
>  extern void gru_flush_all_tlb(struct gru_state *gru);
> --
> 2.25.1
>