[PATCH] misc: hisi_hikey_usb: Fix use after free bug in hisi_hikey_usb_remove due to race condition

[PATCH] misc: hisi_hikey_usb: Fix use after free bug in hisi_hikey_usb_remove due to race condition

[Zheng Wang]

In hisi_hikey_usb_probe, it called hisi_hikey_usb_of_role_switch
and bound &hisi_hikey_usb->work with relay_set_role_switch.
When it calls hub_usb_role_switch_set, it will finally call
schedule_work to start the work.

When we call hisi_hikey_usb_remove to remove the driver, there
may be a sequence as follows:

Fix it by finishing the work before cleanup in hisi_hikey_usb_remove.

CPU0                  CPU1

                    |relay_set_role_switch
hisi_hikey_usb_remove|
  usb_role_switch_put|
    usb_role_switch_release  |
     kfree(sw)     |
                    | usb_role_switch_set_role
                    |   //use

Fixes: 7a6ff4c4cbc3 ("misc: hisi_hikey_usb: Driver to support onboard USB gpio hub on Hikey960")
Signed-off-by: Zheng Wang <zyytlz.wz@163.com>
---
 drivers/misc/hisi_hikey_usb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/misc/hisi_hikey_usb.c b/drivers/misc/hisi_hikey_usb.c
index 2165ec35a343..26fc895c4418 100644
--- a/drivers/misc/hisi_hikey_usb.c
+++ b/drivers/misc/hisi_hikey_usb.c
@@ -242,6 +242,7 @@ static int hisi_hikey_usb_probe(struct platform_device *pdev)
 static int  hisi_hikey_usb_remove(struct platform_device *pdev)
 {
 	struct hisi_hikey_usb *hisi_hikey_usb = platform_get_drvdata(pdev);
+	cancel_work_sync(&hisi_hikey_usb->work);
 
 	if (hisi_hikey_usb->hub_role_sw) {
 		usb_role_switch_unregister(hisi_hikey_usb->hub_role_sw);
-- 
2.25.1

Re: [PATCH] misc: hisi_hikey_usb: Fix use after free bug in hisi_hikey_usb_remove due to race condition

[John Stultz]

On Sun, Mar 12, 2023 at 7:53 AM Zheng Wang <zyytlz.wz@163.com> wrote:
>
> In hisi_hikey_usb_probe, it called hisi_hikey_usb_of_role_switch
> and bound &hisi_hikey_usb->work with relay_set_role_switch.
> When it calls hub_usb_role_switch_set, it will finally call
> schedule_work to start the work.
>
> When we call hisi_hikey_usb_remove to remove the driver, there
> may be a sequence as follows:
>
> Fix it by finishing the work before cleanup in hisi_hikey_usb_remove.
>
> CPU0                  CPU1
>
>                     |relay_set_role_switch
> hisi_hikey_usb_remove|
>   usb_role_switch_put|
>     usb_role_switch_release  |
>      kfree(sw)     |
>                     | usb_role_switch_set_role
>                     |   //use
>
> Fixes: 7a6ff4c4cbc3 ("misc: hisi_hikey_usb: Driver to support onboard USB gpio hub on Hikey960")
> Signed-off-by: Zheng Wang <zyytlz.wz@163.com>
> ---
>  drivers/misc/hisi_hikey_usb.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/misc/hisi_hikey_usb.c b/drivers/misc/hisi_hikey_usb.c
> index 2165ec35a343..26fc895c4418 100644
> --- a/drivers/misc/hisi_hikey_usb.c
> +++ b/drivers/misc/hisi_hikey_usb.c
> @@ -242,6 +242,7 @@ static int hisi_hikey_usb_probe(struct platform_device *pdev)
>  static int  hisi_hikey_usb_remove(struct platform_device *pdev)
>  {
>         struct hisi_hikey_usb *hisi_hikey_usb = platform_get_drvdata(pdev);
> +       cancel_work_sync(&hisi_hikey_usb->work);
>
>         if (hisi_hikey_usb->hub_role_sw) {
>                 usb_role_switch_unregister(hisi_hikey_usb->hub_role_sw);

Looks sane to me.
Pulling in Sumit and YongQin as they have hardware and can test with it.

thanks
-john

Re: [PATCH] misc: hisi_hikey_usb: Fix use after free bug in hisi_hikey_usb_remove due to race condition

[Zheng Hacker]

John Stultz <jstultz@google.com> 于2023年3月14日周二 03:57写道：
>
> On Sun, Mar 12, 2023 at 7:53 AM Zheng Wang <zyytlz.wz@163.com> wrote:
> >
> > In hisi_hikey_usb_probe, it called hisi_hikey_usb_of_role_switch
> > and bound &hisi_hikey_usb->work with relay_set_role_switch.
> > When it calls hub_usb_role_switch_set, it will finally call
> > schedule_work to start the work.
> >
> > When we call hisi_hikey_usb_remove to remove the driver, there
> > may be a sequence as follows:
> >
> > Fix it by finishing the work before cleanup in hisi_hikey_usb_remove.
> >
> > CPU0                  CPU1
> >
> >                     |relay_set_role_switch
> > hisi_hikey_usb_remove|
> >   usb_role_switch_put|
> >     usb_role_switch_release  |
> >      kfree(sw)     |
> >                     | usb_role_switch_set_role
> >                     |   //use
> >
> > Fixes: 7a6ff4c4cbc3 ("misc: hisi_hikey_usb: Driver to support onboard USB gpio hub on Hikey960")
> > Signed-off-by: Zheng Wang <zyytlz.wz@163.com>
> > ---
> >  drivers/misc/hisi_hikey_usb.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/misc/hisi_hikey_usb.c b/drivers/misc/hisi_hikey_usb.c
> > index 2165ec35a343..26fc895c4418 100644
> > --- a/drivers/misc/hisi_hikey_usb.c
> > +++ b/drivers/misc/hisi_hikey_usb.c
> > @@ -242,6 +242,7 @@ static int hisi_hikey_usb_probe(struct platform_device *pdev)
> >  static int  hisi_hikey_usb_remove(struct platform_device *pdev)
> >  {
> >         struct hisi_hikey_usb *hisi_hikey_usb = platform_get_drvdata(pdev);
> > +       cancel_work_sync(&hisi_hikey_usb->work);
> >
> >         if (hisi_hikey_usb->hub_role_sw) {
> >                 usb_role_switch_unregister(hisi_hikey_usb->hub_role_sw);
>
> Looks sane to me.
> Pulling in Sumit and YongQin as they have hardware and can test with it.
>
Hi John,

Thanks for your reply. Thank Sumit and YongQin for being willing to
test the solution with their hardware.

Best regards,
Zheng

Re: [PATCH] misc: hisi_hikey_usb: Fix use after free bug in hisi_hikey_usb_remove due to race condition

[Zheng Hacker]

Friendly ping about the bug.

Thanks,
Zheng

Zheng Hacker <hackerzheng666@gmail.com> 于2023年3月14日周二 09:01写道：
>
> John Stultz <jstultz@google.com> 于2023年3月14日周二 03:57写道：
> >
> > On Sun, Mar 12, 2023 at 7:53 AM Zheng Wang <zyytlz.wz@163.com> wrote:
> > >
> > > In hisi_hikey_usb_probe, it called hisi_hikey_usb_of_role_switch
> > > and bound &hisi_hikey_usb->work with relay_set_role_switch.
> > > When it calls hub_usb_role_switch_set, it will finally call
> > > schedule_work to start the work.
> > >
> > > When we call hisi_hikey_usb_remove to remove the driver, there
> > > may be a sequence as follows:
> > >
> > > Fix it by finishing the work before cleanup in hisi_hikey_usb_remove.
> > >
> > > CPU0                  CPU1
> > >
> > >                     |relay_set_role_switch
> > > hisi_hikey_usb_remove|
> > >   usb_role_switch_put|
> > >     usb_role_switch_release  |
> > >      kfree(sw)     |
> > >                     | usb_role_switch_set_role
> > >                     |   //use
> > >
> > > Fixes: 7a6ff4c4cbc3 ("misc: hisi_hikey_usb: Driver to support onboard USB gpio hub on Hikey960")
> > > Signed-off-by: Zheng Wang <zyytlz.wz@163.com>
> > > ---
> > >  drivers/misc/hisi_hikey_usb.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/misc/hisi_hikey_usb.c b/drivers/misc/hisi_hikey_usb.c
> > > index 2165ec35a343..26fc895c4418 100644
> > > --- a/drivers/misc/hisi_hikey_usb.c
> > > +++ b/drivers/misc/hisi_hikey_usb.c
> > > @@ -242,6 +242,7 @@ static int hisi_hikey_usb_probe(struct platform_device *pdev)
> > >  static int  hisi_hikey_usb_remove(struct platform_device *pdev)
> > >  {
> > >         struct hisi_hikey_usb *hisi_hikey_usb = platform_get_drvdata(pdev);
> > > +       cancel_work_sync(&hisi_hikey_usb->work);
> > >
> > >         if (hisi_hikey_usb->hub_role_sw) {
> > >                 usb_role_switch_unregister(hisi_hikey_usb->hub_role_sw);
> >
> > Looks sane to me.
> > Pulling in Sumit and YongQin as they have hardware and can test with it.
> >
> Hi John,
>
> Thanks for your reply. Thank Sumit and YongQin for being willing to
> test the solution with their hardware.
>
> Best regards,
> Zheng

Re: [PATCH] misc: hisi_hikey_usb: Fix use after free bug in hisi_hikey_usb_remove due to race condition

[Yongqin Liu]

Hi, Zheng

On Thu, 13 Apr 2023 at 16:08, Zheng Hacker <hackerzheng666@gmail.com> wrote:
>
> Friendly ping about the bug.

Sorry, wasn't aware of this message before,

Could you please help share the instructions to reproduce the problem
this change fixes?

Thanks,
Yongqin Liu
> Zheng Hacker <hackerzheng666@gmail.com> 于2023年3月14日周二 09:01写道：
> >
> > John Stultz <jstultz@google.com> 于2023年3月14日周二 03:57写道：
> > >
> > > On Sun, Mar 12, 2023 at 7:53 AM Zheng Wang <zyytlz.wz@163.com> wrote:
> > > >
> > > > In hisi_hikey_usb_probe, it called hisi_hikey_usb_of_role_switch
> > > > and bound &hisi_hikey_usb->work with relay_set_role_switch.
> > > > When it calls hub_usb_role_switch_set, it will finally call
> > > > schedule_work to start the work.
> > > >
> > > > When we call hisi_hikey_usb_remove to remove the driver, there
> > > > may be a sequence as follows:
> > > >
> > > > Fix it by finishing the work before cleanup in hisi_hikey_usb_remove.
> > > >
> > > > CPU0                  CPU1
> > > >
> > > >                     |relay_set_role_switch
> > > > hisi_hikey_usb_remove|
> > > >   usb_role_switch_put|
> > > >     usb_role_switch_release  |
> > > >      kfree(sw)     |
> > > >                     | usb_role_switch_set_role
> > > >                     |   //use
> > > >
> > > > Fixes: 7a6ff4c4cbc3 ("misc: hisi_hikey_usb: Driver to support onboard USB gpio hub on Hikey960")
> > > > Signed-off-by: Zheng Wang <zyytlz.wz@163.com>
> > > > ---
> > > >  drivers/misc/hisi_hikey_usb.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/misc/hisi_hikey_usb.c b/drivers/misc/hisi_hikey_usb.c
> > > > index 2165ec35a343..26fc895c4418 100644
> > > > --- a/drivers/misc/hisi_hikey_usb.c
> > > > +++ b/drivers/misc/hisi_hikey_usb.c
> > > > @@ -242,6 +242,7 @@ static int hisi_hikey_usb_probe(struct platform_device *pdev)
> > > >  static int  hisi_hikey_usb_remove(struct platform_device *pdev)
> > > >  {
> > > >         struct hisi_hikey_usb *hisi_hikey_usb = platform_get_drvdata(pdev);
> > > > +       cancel_work_sync(&hisi_hikey_usb->work);
> > > >
> > > >         if (hisi_hikey_usb->hub_role_sw) {
> > > >                 usb_role_switch_unregister(hisi_hikey_usb->hub_role_sw);
> > >
> > > Looks sane to me.
> > > Pulling in Sumit and YongQin as they have hardware and can test with it.
> > >
> > Hi John,
> >
> > Thanks for your reply. Thank Sumit and YongQin for being willing to
> > test the solution with their hardware.
> >
> > Best regards,
> > Zheng



-- 
Best Regards,
Yongqin Liu
---------------------------------------------------------------
#mailing list
linaro-android@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-android

Re: [PATCH] misc: hisi_hikey_usb: Fix use after free bug in hisi_hikey_usb_remove due to race condition

[Zheng Hacker]

Yongqin Liu <yongqin.liu@linaro.org> 于2023年4月13日周四 18:55写道：
>
> Hi, Zheng
>
> On Thu, 13 Apr 2023 at 16:08, Zheng Hacker <hackerzheng666@gmail.com> wrote:
> >
> > Friendly ping about the bug.
>
> Sorry, wasn't aware of this message before,
>
> Could you please help share the instructions to reproduce the problem
> this change fixes?
>

Hi Yongqin,

Thanks for your reply. This bug is found by static analysis. There is no PoC.

From my personal experience, triggering race condition bugs stably in
the kernel needs some tricks.
For example, you can insert some sleep-time code to slow down the
thread until the related object is freed.
Besides, you can use gdb to control the time window. Also, there are
some other tricks as [1] said.

As for the reproduction, this attack vector requires that the attacker
can physically access the device.
When he/she unplugs the usb, the remove function is triggered, and if
the set callback is invoked, there might be a race condition.

In practice, you can just use rmmod command to simulate the unplug
movement, which will also trigger the hisi_hikey_usb_remove if there
is a real USB device.

If there's some other help I can provide, please feel free to let me know.

Thanks again for your effort.

Best regards,
Zheng

[1] https://www.usenix.org/conference/usenixsecurity21/presentation/lee-yoochan

> Thanks,
> Yongqin Liu
> > Zheng Hacker <hackerzheng666@gmail.com> 于2023年3月14日周二 09:01写道：
> > >
> > > John Stultz <jstultz@google.com> 于2023年3月14日周二 03:57写道：
> > > >
> > > > On Sun, Mar 12, 2023 at 7:53 AM Zheng Wang <zyytlz.wz@163.com> wrote:
> > > > >
> > > > > In hisi_hikey_usb_probe, it called hisi_hikey_usb_of_role_switch
> > > > > and bound &hisi_hikey_usb->work with relay_set_role_switch.
> > > > > When it calls hub_usb_role_switch_set, it will finally call
> > > > > schedule_work to start the work.
> > > > >
> > > > > When we call hisi_hikey_usb_remove to remove the driver, there
> > > > > may be a sequence as follows:
> > > > >
> > > > > Fix it by finishing the work before cleanup in hisi_hikey_usb_remove.
> > > > >
> > > > > CPU0                  CPU1
> > > > >
> > > > >                     |relay_set_role_switch
> > > > > hisi_hikey_usb_remove|
> > > > >   usb_role_switch_put|
> > > > >     usb_role_switch_release  |
> > > > >      kfree(sw)     |
> > > > >                     | usb_role_switch_set_role
> > > > >                     |   //use
> > > > >
> > > > > Fixes: 7a6ff4c4cbc3 ("misc: hisi_hikey_usb: Driver to support onboard USB gpio hub on Hikey960")
> > > > > Signed-off-by: Zheng Wang <zyytlz.wz@163.com>
> > > > > ---
> > > > >  drivers/misc/hisi_hikey_usb.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/drivers/misc/hisi_hikey_usb.c b/drivers/misc/hisi_hikey_usb.c
> > > > > index 2165ec35a343..26fc895c4418 100644
> > > > > --- a/drivers/misc/hisi_hikey_usb.c
> > > > > +++ b/drivers/misc/hisi_hikey_usb.c
> > > > > @@ -242,6 +242,7 @@ static int hisi_hikey_usb_probe(struct platform_device *pdev)
> > > > >  static int  hisi_hikey_usb_remove(struct platform_device *pdev)
> > > > >  {
> > > > >         struct hisi_hikey_usb *hisi_hikey_usb = platform_get_drvdata(pdev);
> > > > > +       cancel_work_sync(&hisi_hikey_usb->work);
> > > > >
> > > > >         if (hisi_hikey_usb->hub_role_sw) {
> > > > >                 usb_role_switch_unregister(hisi_hikey_usb->hub_role_sw);
> > > >
> > > > Looks sane to me.
> > > > Pulling in Sumit and YongQin as they have hardware and can test with it.
> > > >
> > > Hi John,
> > >
> > > Thanks for your reply. Thank Sumit and YongQin for being willing to
> > > test the solution with their hardware.
> > >
> > > Best regards,
> > > Zheng
>
>
>
> --
> Best Regards,
> Yongqin Liu
> ---------------------------------------------------------------
> #mailing list
> linaro-android@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-android

Re: [PATCH] misc: hisi_hikey_usb: Fix use after free bug in hisi_hikey_usb_remove due to race condition

[Greg KH]

On Thu, Apr 13, 2023 at 07:12:07PM +0800, Zheng Hacker wrote:
> Yongqin Liu <yongqin.liu@linaro.org> 于2023年4月13日周四 18:55写道：
> >
> > Hi, Zheng
> >
> > On Thu, 13 Apr 2023 at 16:08, Zheng Hacker <hackerzheng666@gmail.com> wrote:
> > >
> > > Friendly ping about the bug.
> >
> > Sorry, wasn't aware of this message before,
> >
> > Could you please help share the instructions to reproduce the problem
> > this change fixes?
> >
> 
> Hi Yongqin,
> 
> Thanks for your reply. This bug is found by static analysis. There is no PoC.
> 
> >From my personal experience, triggering race condition bugs stably in
> the kernel needs some tricks.
> For example, you can insert some sleep-time code to slow down the
> thread until the related object is freed.
> Besides, you can use gdb to control the time window. Also, there are
> some other tricks as [1] said.
> 
> As for the reproduction, this attack vector requires that the attacker
> can physically access the device.
> When he/she unplugs the usb, the remove function is triggered, and if
> the set callback is invoked, there might be a race condition.

How does the removal of the USB device trigger a platform device
removal?

Are you sure this can be triggered by some other way other than manually
unloading the driver?

thanks,

greg k-h

Re: [PATCH] misc: hisi_hikey_usb: Fix use after free bug in hisi_hikey_usb_remove due to race condition

[Zheng Hacker]

Greg KH <gregkh@linuxfoundation.org> 于2023年4月13日周四 20:48写道：
>
> On Thu, Apr 13, 2023 at 07:12:07PM +0800, Zheng Hacker wrote:
> > Yongqin Liu <yongqin.liu@linaro.org> 于2023年4月13日周四 18:55写道：
> > >
> > > Hi, Zheng
> > >
> > > On Thu, 13 Apr 2023 at 16:08, Zheng Hacker <hackerzheng666@gmail.com> wrote:
> > > >
> > > > Friendly ping about the bug.
> > >
> > > Sorry, wasn't aware of this message before,
> > >
> > > Could you please help share the instructions to reproduce the problem
> > > this change fixes?
> > >
> >
> > Hi Yongqin,
> >
> > Thanks for your reply. This bug is found by static analysis. There is no PoC.
> >
> > >From my personal experience, triggering race condition bugs stably in
> > the kernel needs some tricks.
> > For example, you can insert some sleep-time code to slow down the
> > thread until the related object is freed.
> > Besides, you can use gdb to control the time window. Also, there are
> > some other tricks as [1] said.
> >
> > As for the reproduction, this attack vector requires that the attacker
> > can physically access the device.
> > When he/she unplugs the usb, the remove function is triggered, and if
> > the set callback is invoked, there might be a race condition.
>
> How does the removal of the USB device trigger a platform device
> removal?

Sorry I made a mistake. The USB device usually calls disconnect
callback when it's unpluged.
What I want to express here is When the driver-related device(here
it's USB GPIO Hub) was removed, the remove function is triggered.
>
> Are you sure this can be triggered by some other way other than manually
> unloading the driver?

As I didn't make a PoC, I'm not 100 percent sure about that.

Best regards,
Zheng

>
> thanks,
>
> greg k-h

Re: [PATCH] misc: hisi_hikey_usb: Fix use after free bug in hisi_hikey_usb_remove due to race condition

[Greg KH]

On Thu, Apr 13, 2023 at 11:35:17PM +0800, Zheng Hacker wrote:
> Greg KH <gregkh@linuxfoundation.org> 于2023年4月13日周四 20:48写道：
> >
> > On Thu, Apr 13, 2023 at 07:12:07PM +0800, Zheng Hacker wrote:
> > > Yongqin Liu <yongqin.liu@linaro.org> 于2023年4月13日周四 18:55写道：
> > > >
> > > > Hi, Zheng
> > > >
> > > > On Thu, 13 Apr 2023 at 16:08, Zheng Hacker <hackerzheng666@gmail.com> wrote:
> > > > >
> > > > > Friendly ping about the bug.
> > > >
> > > > Sorry, wasn't aware of this message before,
> > > >
> > > > Could you please help share the instructions to reproduce the problem
> > > > this change fixes?
> > > >
> > >
> > > Hi Yongqin,
> > >
> > > Thanks for your reply. This bug is found by static analysis. There is no PoC.
> > >
> > > >From my personal experience, triggering race condition bugs stably in
> > > the kernel needs some tricks.
> > > For example, you can insert some sleep-time code to slow down the
> > > thread until the related object is freed.
> > > Besides, you can use gdb to control the time window. Also, there are
> > > some other tricks as [1] said.
> > >
> > > As for the reproduction, this attack vector requires that the attacker
> > > can physically access the device.
> > > When he/she unplugs the usb, the remove function is triggered, and if
> > > the set callback is invoked, there might be a race condition.
> >
> > How does the removal of the USB device trigger a platform device
> > removal?
> 
> Sorry I made a mistake. The USB device usually calls disconnect
> callback when it's unpluged.

Yes, but you are changing the platform device disconnect, not the USB
device disconnect.

> What I want to express here is When the driver-related device(here
> it's USB GPIO Hub) was removed, the remove function is triggered.

And is this a patform device on a USB device?  If so, that's a bigger
problem that we need to fix as that is not allowed.

But in looking at the code, it does not seem to be that at all, this is
just a "normal" platform device.  So how can it ever be removed from the
system?  (and no, unloading the driver doesn't count, that can never
happen on a normal machine.)

thanks,

greg k-h

Re: [PATCH] misc: hisi_hikey_usb: Fix use after free bug in hisi_hikey_usb_remove due to race condition

[Zheng Hacker]

Greg KH <gregkh@linuxfoundation.org> 于2023年4月13日周四 23:56写道：
>
> On Thu, Apr 13, 2023 at 11:35:17PM +0800, Zheng Hacker wrote:
> > Greg KH <gregkh@linuxfoundation.org> 于2023年4月13日周四 20:48写道：
> > >
> > > On Thu, Apr 13, 2023 at 07:12:07PM +0800, Zheng Hacker wrote:
> > > > Yongqin Liu <yongqin.liu@linaro.org> 于2023年4月13日周四 18:55写道：
> > > > >
> > > > > Hi, Zheng
> > > > >
> > > > > On Thu, 13 Apr 2023 at 16:08, Zheng Hacker <hackerzheng666@gmail.com> wrote:
> > > > > >
> > > > > > Friendly ping about the bug.
> > > > >
> > > > > Sorry, wasn't aware of this message before,
> > > > >
> > > > > Could you please help share the instructions to reproduce the problem
> > > > > this change fixes?
> > > > >
> > > >
> > > > Hi Yongqin,
> > > >
> > > > Thanks for your reply. This bug is found by static analysis. There is no PoC.
> > > >
> > > > >From my personal experience, triggering race condition bugs stably in
> > > > the kernel needs some tricks.
> > > > For example, you can insert some sleep-time code to slow down the
> > > > thread until the related object is freed.
> > > > Besides, you can use gdb to control the time window. Also, there are
> > > > some other tricks as [1] said.
> > > >
> > > > As for the reproduction, this attack vector requires that the attacker
> > > > can physically access the device.
> > > > When he/she unplugs the usb, the remove function is triggered, and if
> > > > the set callback is invoked, there might be a race condition.
> > >
> > > How does the removal of the USB device trigger a platform device
> > > removal?
> >
> > Sorry I made a mistake. The USB device usually calls disconnect
> > callback when it's unpluged.
>
> Yes, but you are changing the platform device disconnect, not the USB
> device disconnect.
>
> > What I want to express here is When the driver-related device(here
> > it's USB GPIO Hub) was removed, the remove function is triggered.
>
> And is this a patform device on a USB device?  If so, that's a bigger
> problem that we need to fix as that is not allowed.

No this is not a platform  device on a USB device.

>
> But in looking at the code, it does not seem to be that at all, this is
> just a "normal" platform device.  So how can it ever be removed from the
> system?  (and no, unloading the driver doesn't count, that can never
> happen on a normal machine.)
>

Yes, I finally figured out your meaning. I know it's hard to unplug
the platform device
directly. All I want to express is that it's a theoretical method
except  rmmod. I think it's better to fix the bug. But if the
developers think it's practically impossible, I think there's no need
to take further action.

Sorry for wasting your time and thanks for your explanation.

Best regards,
Zheng

> thanks,
>
> greg k-h

Re: [PATCH] misc: hisi_hikey_usb: Fix use after free bug in hisi_hikey_usb_remove due to race condition

[Yongqin Liu]

Hi, Zheng

Sorry for the late reply.

I tested this change with Android build based on the ACK
android-mainline branch.
The hisi_hikey_usb module could not be removed with error like this:
    console:/ # rmmod -f hisi_hikey_usb
    rmmod: failed to unload hisi_hikey_usb: Try again
    1|console:/ #
Sorry I am not able to reproduce any problem without this commit,
but I do not see any problem with this change applied either.

If there is any specific things you want to check, please feel free let me know

Thanks,
Yongqin Liu


On Fri, 14 Apr 2023 at 00:46, Zheng Hacker <hackerzheng666@gmail.com> wrote:
>
> Greg KH <gregkh@linuxfoundation.org> 于2023年4月13日周四 23:56写道：
> >
> > On Thu, Apr 13, 2023 at 11:35:17PM +0800, Zheng Hacker wrote:
> > > Greg KH <gregkh@linuxfoundation.org> 于2023年4月13日周四 20:48写道：
> > > >
> > > > On Thu, Apr 13, 2023 at 07:12:07PM +0800, Zheng Hacker wrote:
> > > > > Yongqin Liu <yongqin.liu@linaro.org> 于2023年4月13日周四 18:55写道：
> > > > > >
> > > > > > Hi, Zheng
> > > > > >
> > > > > > On Thu, 13 Apr 2023 at 16:08, Zheng Hacker <hackerzheng666@gmail.com> wrote:
> > > > > > >
> > > > > > > Friendly ping about the bug.
> > > > > >
> > > > > > Sorry, wasn't aware of this message before,
> > > > > >
> > > > > > Could you please help share the instructions to reproduce the problem
> > > > > > this change fixes?
> > > > > >
> > > > >
> > > > > Hi Yongqin,
> > > > >
> > > > > Thanks for your reply. This bug is found by static analysis. There is no PoC.
> > > > >
> > > > > >From my personal experience, triggering race condition bugs stably in
> > > > > the kernel needs some tricks.
> > > > > For example, you can insert some sleep-time code to slow down the
> > > > > thread until the related object is freed.
> > > > > Besides, you can use gdb to control the time window. Also, there are
> > > > > some other tricks as [1] said.
> > > > >
> > > > > As for the reproduction, this attack vector requires that the attacker
> > > > > can physically access the device.
> > > > > When he/she unplugs the usb, the remove function is triggered, and if
> > > > > the set callback is invoked, there might be a race condition.
> > > >
> > > > How does the removal of the USB device trigger a platform device
> > > > removal?
> > >
> > > Sorry I made a mistake. The USB device usually calls disconnect
> > > callback when it's unpluged.
> >
> > Yes, but you are changing the platform device disconnect, not the USB
> > device disconnect.
> >
> > > What I want to express here is When the driver-related device(here
> > > it's USB GPIO Hub) was removed, the remove function is triggered.
> >
> > And is this a patform device on a USB device?  If so, that's a bigger
> > problem that we need to fix as that is not allowed.
>
> No this is not a platform  device on a USB device.
>
> >
> > But in looking at the code, it does not seem to be that at all, this is
> > just a "normal" platform device.  So how can it ever be removed from the
> > system?  (and no, unloading the driver doesn't count, that can never
> > happen on a normal machine.)
> >
>
> Yes, I finally figured out your meaning. I know it's hard to unplug
> the platform device
> directly. All I want to express is that it's a theoretical method
> except  rmmod. I think it's better to fix the bug. But if the
> developers think it's practically impossible, I think there's no need
> to take further action.
>
> Sorry for wasting your time and thanks for your explanation.
>
> Best regards,
> Zheng
>
> > thanks,
> >
> > greg k-h



--
Best Regards,
Yongqin Liu
---------------------------------------------------------------
#mailing list
linaro-android@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-android

Re: [PATCH] misc: hisi_hikey_usb: Fix use after free bug in hisi_hikey_usb_remove due to race condition

[Zheng Hacker]

Yongqin Liu <yongqin.liu@linaro.org> 于2023年4月18日周二 01:31写道：
>
> Hi, Zheng
>
> Sorry for the late reply.
>
> I tested this change with Android build based on the ACK
> android-mainline branch.
> The hisi_hikey_usb module could not be removed with error like this:
>     console:/ # rmmod -f hisi_hikey_usb
>     rmmod: failed to unload hisi_hikey_usb: Try again
>     1|console:/ #
> Sorry I am not able to reproduce any problem without this commit,
> but I do not see any problem with this change applied either.
>
> If there is any specific things you want to check, please feel free let me know
>

Hi Yongqin,

Thanks for your testing. I have no more questions about the issue.

Best regards,
Zheng

> Thanks,
> Yongqin Liu
>
>
> On Fri, 14 Apr 2023 at 00:46, Zheng Hacker <hackerzheng666@gmail.com> wrote:
> >
> > Greg KH <gregkh@linuxfoundation.org> 于2023年4月13日周四 23:56写道：
> > >
> > > On Thu, Apr 13, 2023 at 11:35:17PM +0800, Zheng Hacker wrote:
> > > > Greg KH <gregkh@linuxfoundation.org> 于2023年4月13日周四 20:48写道：
> > > > >
> > > > > On Thu, Apr 13, 2023 at 07:12:07PM +0800, Zheng Hacker wrote:
> > > > > > Yongqin Liu <yongqin.liu@linaro.org> 于2023年4月13日周四 18:55写道：
> > > > > > >
> > > > > > > Hi, Zheng
> > > > > > >
> > > > > > > On Thu, 13 Apr 2023 at 16:08, Zheng Hacker <hackerzheng666@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Friendly ping about the bug.
> > > > > > >
> > > > > > > Sorry, wasn't aware of this message before,
> > > > > > >
> > > > > > > Could you please help share the instructions to reproduce the problem
> > > > > > > this change fixes?
> > > > > > >
> > > > > >
> > > > > > Hi Yongqin,
> > > > > >
> > > > > > Thanks for your reply. This bug is found by static analysis. There is no PoC.
> > > > > >
> > > > > > >From my personal experience, triggering race condition bugs stably in
> > > > > > the kernel needs some tricks.
> > > > > > For example, you can insert some sleep-time code to slow down the
> > > > > > thread until the related object is freed.
> > > > > > Besides, you can use gdb to control the time window. Also, there are
> > > > > > some other tricks as [1] said.
> > > > > >
> > > > > > As for the reproduction, this attack vector requires that the attacker
> > > > > > can physically access the device.
> > > > > > When he/she unplugs the usb, the remove function is triggered, and if
> > > > > > the set callback is invoked, there might be a race condition.
> > > > >
> > > > > How does the removal of the USB device trigger a platform device
> > > > > removal?
> > > >
> > > > Sorry I made a mistake. The USB device usually calls disconnect
> > > > callback when it's unpluged.
> > >
> > > Yes, but you are changing the platform device disconnect, not the USB
> > > device disconnect.
> > >
> > > > What I want to express here is When the driver-related device(here
> > > > it's USB GPIO Hub) was removed, the remove function is triggered.
> > >
> > > And is this a patform device on a USB device?  If so, that's a bigger
> > > problem that we need to fix as that is not allowed.
> >
> > No this is not a platform  device on a USB device.
> >
> > >
> > > But in looking at the code, it does not seem to be that at all, this is
> > > just a "normal" platform device.  So how can it ever be removed from the
> > > system?  (and no, unloading the driver doesn't count, that can never
> > > happen on a normal machine.)
> > >
> >
> > Yes, I finally figured out your meaning. I know it's hard to unplug
> > the platform device
> > directly. All I want to express is that it's a theoretical method
> > except  rmmod. I think it's better to fix the bug. But if the
> > developers think it's practically impossible, I think there's no need
> > to take further action.
> >
> > Sorry for wasting your time and thanks for your explanation.
> >
> > Best regards,
> > Zheng
> >
> > > thanks,
> > >
> > > greg k-h
>
>
>
> --
> Best Regards,
> Yongqin Liu
> ---------------------------------------------------------------
> #mailing list
> linaro-android@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-android

Re: [PATCH] misc: hisi_hikey_usb: Fix use after free bug in hisi_hikey_usb_remove due to race condition

[Yongqin Liu]

Hi, Zheng

BTW, I just see cancel_delayed_work_sync is used in
the drivers/usb/common/usb-conn-gpio.c usb_conn_remove function.
    https://android.googlesource.com/kernel/common/+/refs/heads/android-mainline/drivers/usb/common/usb-conn-gpio.c#274

I know nothing about the cancel_delayed_work_sync and cancel_work_sync
functions,
just for your information in case cancel_delayed_work_sync might be
better to be used here.

Thanks,
Yongqin Liu
On Tue, 18 Apr 2023 at 21:18, Zheng Hacker <hackerzheng666@gmail.com> wrote:
>
> Yongqin Liu <yongqin.liu@linaro.org> 于2023年4月18日周二 01:31写道：
> >
> > Hi, Zheng
> >
> > Sorry for the late reply.
> >
> > I tested this change with Android build based on the ACK
> > android-mainline branch.
> > The hisi_hikey_usb module could not be removed with error like this:
> >     console:/ # rmmod -f hisi_hikey_usb
> >     rmmod: failed to unload hisi_hikey_usb: Try again
> >     1|console:/ #
> > Sorry I am not able to reproduce any problem without this commit,
> > but I do not see any problem with this change applied either.
> >
> > If there is any specific things you want to check, please feel free let me know
> >
>
> Hi Yongqin,
>
> Thanks for your testing. I have no more questions about the issue.
>
> Best regards,
> Zheng
>
> > Thanks,
> > Yongqin Liu
> >
> >
> > On Fri, 14 Apr 2023 at 00:46, Zheng Hacker <hackerzheng666@gmail.com> wrote:
> > >
> > > Greg KH <gregkh@linuxfoundation.org> 于2023年4月13日周四 23:56写道：
> > > >
> > > > On Thu, Apr 13, 2023 at 11:35:17PM +0800, Zheng Hacker wrote:
> > > > > Greg KH <gregkh@linuxfoundation.org> 于2023年4月13日周四 20:48写道：
> > > > > >
> > > > > > On Thu, Apr 13, 2023 at 07:12:07PM +0800, Zheng Hacker wrote:
> > > > > > > Yongqin Liu <yongqin.liu@linaro.org> 于2023年4月13日周四 18:55写道：
> > > > > > > >
> > > > > > > > Hi, Zheng
> > > > > > > >
> > > > > > > > On Thu, 13 Apr 2023 at 16:08, Zheng Hacker <hackerzheng666@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > Friendly ping about the bug.
> > > > > > > >
> > > > > > > > Sorry, wasn't aware of this message before,
> > > > > > > >
> > > > > > > > Could you please help share the instructions to reproduce the problem
> > > > > > > > this change fixes?
> > > > > > > >
> > > > > > >
> > > > > > > Hi Yongqin,
> > > > > > >
> > > > > > > Thanks for your reply. This bug is found by static analysis. There is no PoC.
> > > > > > >
> > > > > > > >From my personal experience, triggering race condition bugs stably in
> > > > > > > the kernel needs some tricks.
> > > > > > > For example, you can insert some sleep-time code to slow down the
> > > > > > > thread until the related object is freed.
> > > > > > > Besides, you can use gdb to control the time window. Also, there are
> > > > > > > some other tricks as [1] said.
> > > > > > >
> > > > > > > As for the reproduction, this attack vector requires that the attacker
> > > > > > > can physically access the device.
> > > > > > > When he/she unplugs the usb, the remove function is triggered, and if
> > > > > > > the set callback is invoked, there might be a race condition.
> > > > > >
> > > > > > How does the removal of the USB device trigger a platform device
> > > > > > removal?
> > > > >
> > > > > Sorry I made a mistake. The USB device usually calls disconnect
> > > > > callback when it's unpluged.
> > > >
> > > > Yes, but you are changing the platform device disconnect, not the USB
> > > > device disconnect.
> > > >
> > > > > What I want to express here is When the driver-related device(here
> > > > > it's USB GPIO Hub) was removed, the remove function is triggered.
> > > >
> > > > And is this a patform device on a USB device?  If so, that's a bigger
> > > > problem that we need to fix as that is not allowed.
> > >
> > > No this is not a platform  device on a USB device.
> > >
> > > >
> > > > But in looking at the code, it does not seem to be that at all, this is
> > > > just a "normal" platform device.  So how can it ever be removed from the
> > > > system?  (and no, unloading the driver doesn't count, that can never
> > > > happen on a normal machine.)
> > > >
> > >
> > > Yes, I finally figured out your meaning. I know it's hard to unplug
> > > the platform device
> > > directly. All I want to express is that it's a theoretical method
> > > except  rmmod. I think it's better to fix the bug. But if the
> > > developers think it's practically impossible, I think there's no need
> > > to take further action.
> > >
> > > Sorry for wasting your time and thanks for your explanation.
> > >
> > > Best regards,
> > > Zheng
> > >
> > > > thanks,
> > > >
> > > > greg k-h
> >
> >
> >
> > --
> > Best Regards,
> > Yongqin Liu
> > ---------------------------------------------------------------
> > #mailing list
> > linaro-android@lists.linaro.org
> > http://lists.linaro.org/mailman/listinfo/linaro-android



-- 
Best Regards,
Yongqin Liu
---------------------------------------------------------------
#mailing list
linaro-android@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-android

Re: [PATCH] misc: hisi_hikey_usb: Fix use after free bug in hisi_hikey_usb_remove due to race condition

[Zheng Hacker]

Yongqin Liu <yongqin.liu@linaro.org> 于2023年4月20日周四 14:31写道：
>
> Hi, Zheng
>
> BTW, I just see cancel_delayed_work_sync is used in
> the drivers/usb/common/usb-conn-gpio.c usb_conn_remove function.
>     https://android.googlesource.com/kernel/common/+/refs/heads/android-mainline/drivers/usb/common/usb-conn-gpio.c#274
>
> I know nothing about the cancel_delayed_work_sync and cancel_work_sync
> functions,
> just for your information in case cancel_delayed_work_sync might be
> better to be used here.
>

HI Yongqin,

Thanks for your kind reminder. The cancel_delayed_work_sync is used
with INIT_DELAYED_WORK and queue_delayed_work.
This is used to start a work after some time while schedule_work means
start it immediately.

In this case, the &hisi_hikey_usb->work is initialized with INIT_WORK
and scheduled with schedule_work. So I think we'd better use
cancel_work_sync here. I'm also not so familiar with the code. If
there's something wrong with it, please feel free to let me know.

Best regards,
Zheng


> Thanks,
> Yongqin Liu
> On Tue, 18 Apr 2023 at 21:18, Zheng Hacker <hackerzheng666@gmail.com> wrote:
> >
> > Yongqin Liu <yongqin.liu@linaro.org> 于2023年4月18日周二 01:31写道：
> > >
> > > Hi, Zheng
> > >
> > > Sorry for the late reply.
> > >
> > > I tested this change with Android build based on the ACK
> > > android-mainline branch.
> > > The hisi_hikey_usb module could not be removed with error like this:
> > >     console:/ # rmmod -f hisi_hikey_usb
> > >     rmmod: failed to unload hisi_hikey_usb: Try again
> > >     1|console:/ #
> > > Sorry I am not able to reproduce any problem without this commit,
> > > but I do not see any problem with this change applied either.
> > >
> > > If there is any specific things you want to check, please feel free let me know
> > >
> >
> > Hi Yongqin,
> >
> > Thanks for your testing. I have no more questions about the issue.
> >
> > Best regards,
> > Zheng
> >
> > > Thanks,
> > > Yongqin Liu
> > >
> > >
> > > On Fri, 14 Apr 2023 at 00:46, Zheng Hacker <hackerzheng666@gmail.com> wrote:
> > > >
> > > > Greg KH <gregkh@linuxfoundation.org> 于2023年4月13日周四 23:56写道：
> > > > >
> > > > > On Thu, Apr 13, 2023 at 11:35:17PM +0800, Zheng Hacker wrote:
> > > > > > Greg KH <gregkh@linuxfoundation.org> 于2023年4月13日周四 20:48写道：
> > > > > > >
> > > > > > > On Thu, Apr 13, 2023 at 07:12:07PM +0800, Zheng Hacker wrote:
> > > > > > > > Yongqin Liu <yongqin.liu@linaro.org> 于2023年4月13日周四 18:55写道：
> > > > > > > > >
> > > > > > > > > Hi, Zheng
> > > > > > > > >
> > > > > > > > > On Thu, 13 Apr 2023 at 16:08, Zheng Hacker <hackerzheng666@gmail.com> wrote:
> > > > > > > > > >
> > > > > > > > > > Friendly ping about the bug.
> > > > > > > > >
> > > > > > > > > Sorry, wasn't aware of this message before,
> > > > > > > > >
> > > > > > > > > Could you please help share the instructions to reproduce the problem
> > > > > > > > > this change fixes?
> > > > > > > > >
> > > > > > > >
> > > > > > > > Hi Yongqin,
> > > > > > > >
> > > > > > > > Thanks for your reply. This bug is found by static analysis. There is no PoC.
> > > > > > > >
> > > > > > > > >From my personal experience, triggering race condition bugs stably in
> > > > > > > > the kernel needs some tricks.
> > > > > > > > For example, you can insert some sleep-time code to slow down the
> > > > > > > > thread until the related object is freed.
> > > > > > > > Besides, you can use gdb to control the time window. Also, there are
> > > > > > > > some other tricks as [1] said.
> > > > > > > >
> > > > > > > > As for the reproduction, this attack vector requires that the attacker
> > > > > > > > can physically access the device.
> > > > > > > > When he/she unplugs the usb, the remove function is triggered, and if
> > > > > > > > the set callback is invoked, there might be a race condition.
> > > > > > >
> > > > > > > How does the removal of the USB device trigger a platform device
> > > > > > > removal?
> > > > > >
> > > > > > Sorry I made a mistake. The USB device usually calls disconnect
> > > > > > callback when it's unpluged.
> > > > >
> > > > > Yes, but you are changing the platform device disconnect, not the USB
> > > > > device disconnect.
> > > > >
> > > > > > What I want to express here is When the driver-related device(here
> > > > > > it's USB GPIO Hub) was removed, the remove function is triggered.
> > > > >
> > > > > And is this a patform device on a USB device?  If so, that's a bigger
> > > > > problem that we need to fix as that is not allowed.
> > > >
> > > > No this is not a platform  device on a USB device.
> > > >
> > > > >
> > > > > But in looking at the code, it does not seem to be that at all, this is
> > > > > just a "normal" platform device.  So how can it ever be removed from the
> > > > > system?  (and no, unloading the driver doesn't count, that can never
> > > > > happen on a normal machine.)
> > > > >
> > > >
> > > > Yes, I finally figured out your meaning. I know it's hard to unplug
> > > > the platform device
> > > > directly. All I want to express is that it's a theoretical method
> > > > except  rmmod. I think it's better to fix the bug. But if the
> > > > developers think it's practically impossible, I think there's no need
> > > > to take further action.
> > > >
> > > > Sorry for wasting your time and thanks for your explanation.
> > > >
> > > > Best regards,
> > > > Zheng
> > > >
> > > > > thanks,
> > > > >
> > > > > greg k-h
> > >
> > >
> > >
> > > --
> > > Best Regards,
> > > Yongqin Liu
> > > ---------------------------------------------------------------
> > > #mailing list
> > > linaro-android@lists.linaro.org
> > > http://lists.linaro.org/mailman/listinfo/linaro-android
>
>
>
> --
> Best Regards,
> Yongqin Liu
> ---------------------------------------------------------------
> #mailing list
> linaro-android@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-android

Re: [PATCH] misc: hisi_hikey_usb: Fix use after free bug in hisi_hikey_usb_remove due to race condition

[Yongqin Liu]

HI, Zheng

Thanks for the explanation!
BTW, from what I tested, I am OK to have this change merged.

Thanks,
Yongqin Liu
On Fri, 21 Apr 2023 at 10:35, Zheng Hacker <hackerzheng666@gmail.com> wrote:
>
> Yongqin Liu <yongqin.liu@linaro.org> 于2023年4月20日周四 14:31写道：
> >
> > Hi, Zheng
> >
> > BTW, I just see cancel_delayed_work_sync is used in
> > the drivers/usb/common/usb-conn-gpio.c usb_conn_remove function.
> >     https://android.googlesource.com/kernel/common/+/refs/heads/android-mainline/drivers/usb/common/usb-conn-gpio.c#274
> >
> > I know nothing about the cancel_delayed_work_sync and cancel_work_sync
> > functions,
> > just for your information in case cancel_delayed_work_sync might be
> > better to be used here.
> >
>
> HI Yongqin,
>
> Thanks for your kind reminder. The cancel_delayed_work_sync is used
> with INIT_DELAYED_WORK and queue_delayed_work.
> This is used to start a work after some time while schedule_work means
> start it immediately.
>
> In this case, the &hisi_hikey_usb->work is initialized with INIT_WORK
> and scheduled with schedule_work. So I think we'd better use
> cancel_work_sync here. I'm also not so familiar with the code. If
> there's something wrong with it, please feel free to let me know.
>
> Best regards,
> Zheng
>
>
> > Thanks,
> > Yongqin Liu
> > On Tue, 18 Apr 2023 at 21:18, Zheng Hacker <hackerzheng666@gmail.com> wrote:
> > >
> > > Yongqin Liu <yongqin.liu@linaro.org> 于2023年4月18日周二 01:31写道：
> > > >
> > > > Hi, Zheng
> > > >
> > > > Sorry for the late reply.
> > > >
> > > > I tested this change with Android build based on the ACK
> > > > android-mainline branch.
> > > > The hisi_hikey_usb module could not be removed with error like this:
> > > >     console:/ # rmmod -f hisi_hikey_usb
> > > >     rmmod: failed to unload hisi_hikey_usb: Try again
> > > >     1|console:/ #
> > > > Sorry I am not able to reproduce any problem without this commit,
> > > > but I do not see any problem with this change applied either.
> > > >
> > > > If there is any specific things you want to check, please feel free let me know
> > > >
> > >
> > > Hi Yongqin,
> > >
> > > Thanks for your testing. I have no more questions about the issue.
> > >
> > > Best regards,
> > > Zheng
> > >
> > > > Thanks,
> > > > Yongqin Liu
> > > >
> > > >
> > > > On Fri, 14 Apr 2023 at 00:46, Zheng Hacker <hackerzheng666@gmail.com> wrote:
> > > > >
> > > > > Greg KH <gregkh@linuxfoundation.org> 于2023年4月13日周四 23:56写道：
> > > > > >
> > > > > > On Thu, Apr 13, 2023 at 11:35:17PM +0800, Zheng Hacker wrote:
> > > > > > > Greg KH <gregkh@linuxfoundation.org> 于2023年4月13日周四 20:48写道：
> > > > > > > >
> > > > > > > > On Thu, Apr 13, 2023 at 07:12:07PM +0800, Zheng Hacker wrote:
> > > > > > > > > Yongqin Liu <yongqin.liu@linaro.org> 于2023年4月13日周四 18:55写道：
> > > > > > > > > >
> > > > > > > > > > Hi, Zheng
> > > > > > > > > >
> > > > > > > > > > On Thu, 13 Apr 2023 at 16:08, Zheng Hacker <hackerzheng666@gmail.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Friendly ping about the bug.
> > > > > > > > > >
> > > > > > > > > > Sorry, wasn't aware of this message before,
> > > > > > > > > >
> > > > > > > > > > Could you please help share the instructions to reproduce the problem
> > > > > > > > > > this change fixes?
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Hi Yongqin,
> > > > > > > > >
> > > > > > > > > Thanks for your reply. This bug is found by static analysis. There is no PoC.
> > > > > > > > >
> > > > > > > > > >From my personal experience, triggering race condition bugs stably in
> > > > > > > > > the kernel needs some tricks.
> > > > > > > > > For example, you can insert some sleep-time code to slow down the
> > > > > > > > > thread until the related object is freed.
> > > > > > > > > Besides, you can use gdb to control the time window. Also, there are
> > > > > > > > > some other tricks as [1] said.
> > > > > > > > >
> > > > > > > > > As for the reproduction, this attack vector requires that the attacker
> > > > > > > > > can physically access the device.
> > > > > > > > > When he/she unplugs the usb, the remove function is triggered, and if
> > > > > > > > > the set callback is invoked, there might be a race condition.
> > > > > > > >
> > > > > > > > How does the removal of the USB device trigger a platform device
> > > > > > > > removal?
> > > > > > >
> > > > > > > Sorry I made a mistake. The USB device usually calls disconnect
> > > > > > > callback when it's unpluged.
> > > > > >
> > > > > > Yes, but you are changing the platform device disconnect, not the USB
> > > > > > device disconnect.
> > > > > >
> > > > > > > What I want to express here is When the driver-related device(here
> > > > > > > it's USB GPIO Hub) was removed, the remove function is triggered.
> > > > > >
> > > > > > And is this a patform device on a USB device?  If so, that's a bigger
> > > > > > problem that we need to fix as that is not allowed.
> > > > >
> > > > > No this is not a platform  device on a USB device.
> > > > >
> > > > > >
> > > > > > But in looking at the code, it does not seem to be that at all, this is
> > > > > > just a "normal" platform device.  So how can it ever be removed from the
> > > > > > system?  (and no, unloading the driver doesn't count, that can never
> > > > > > happen on a normal machine.)
> > > > > >
> > > > >
> > > > > Yes, I finally figured out your meaning. I know it's hard to unplug
> > > > > the platform device
> > > > > directly. All I want to express is that it's a theoretical method
> > > > > except  rmmod. I think it's better to fix the bug. But if the
> > > > > developers think it's practically impossible, I think there's no need
> > > > > to take further action.
> > > > >
> > > > > Sorry for wasting your time and thanks for your explanation.
> > > > >
> > > > > Best regards,
> > > > > Zheng
> > > > >
> > > > > > thanks,
> > > > > >
> > > > > > greg k-h
> > > >
> > > >
> > > >
> > > > --
> > > > Best Regards,
> > > > Yongqin Liu
> > > > ---------------------------------------------------------------
> > > > #mailing list
> > > > linaro-android@lists.linaro.org
> > > > http://lists.linaro.org/mailman/listinfo/linaro-android
> >
> >
> >
> > --
> > Best Regards,
> > Yongqin Liu
> > ---------------------------------------------------------------
> > #mailing list
> > linaro-android@lists.linaro.org
> > http://lists.linaro.org/mailman/listinfo/linaro-android



-- 
Best Regards,
Yongqin Liu
---------------------------------------------------------------
#mailing list
linaro-android@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-android

Re: [PATCH] misc: hisi_hikey_usb: Fix use after free bug in hisi_hikey_usb_remove due to race condition

[Zheng Hacker]

Yongqin Liu <yongqin.liu@linaro.org> 于2023年4月21日周五 23:42写道：
>
> HI, Zheng
>
> Thanks for the explanation!
> BTW, from what I tested, I am OK to have this change merged.
>

Hi Yongqin,

Thanks for your testing and review.

Best regards,
Zheng

> Thanks,
> Yongqin Liu
> On Fri, 21 Apr 2023 at 10:35, Zheng Hacker <hackerzheng666@gmail.com> wrote:
> >
> > Yongqin Liu <yongqin.liu@linaro.org> 于2023年4月20日周四 14:31写道：
> > >
> > > Hi, Zheng
> > >
> > > BTW, I just see cancel_delayed_work_sync is used in
> > > the drivers/usb/common/usb-conn-gpio.c usb_conn_remove function.
> > >     https://android.googlesource.com/kernel/common/+/refs/heads/android-mainline/drivers/usb/common/usb-conn-gpio.c#274
> > >
> > > I know nothing about the cancel_delayed_work_sync and cancel_work_sync
> > > functions,
> > > just for your information in case cancel_delayed_work_sync might be
> > > better to be used here.
> > >
> >
> > HI Yongqin,
> >
> > Thanks for your kind reminder. The cancel_delayed_work_sync is used
> > with INIT_DELAYED_WORK and queue_delayed_work.
> > This is used to start a work after some time while schedule_work means
> > start it immediately.
> >
> > In this case, the &hisi_hikey_usb->work is initialized with INIT_WORK
> > and scheduled with schedule_work. So I think we'd better use
> > cancel_work_sync here. I'm also not so familiar with the code. If
> > there's something wrong with it, please feel free to let me know.
> >
> > Best regards,
> > Zheng
> >
> >
> > > Thanks,
> > > Yongqin Liu
> > > On Tue, 18 Apr 2023 at 21:18, Zheng Hacker <hackerzheng666@gmail.com> wrote:
> > > >
> > > > Yongqin Liu <yongqin.liu@linaro.org> 于2023年4月18日周二 01:31写道：
> > > > >
> > > > > Hi, Zheng
> > > > >
> > > > > Sorry for the late reply.
> > > > >
> > > > > I tested this change with Android build based on the ACK
> > > > > android-mainline branch.
> > > > > The hisi_hikey_usb module could not be removed with error like this:
> > > > >     console:/ # rmmod -f hisi_hikey_usb
> > > > >     rmmod: failed to unload hisi_hikey_usb: Try again
> > > > >     1|console:/ #
> > > > > Sorry I am not able to reproduce any problem without this commit,
> > > > > but I do not see any problem with this change applied either.
> > > > >
> > > > > If there is any specific things you want to check, please feel free let me know
> > > > >
> > > >
> > > > Hi Yongqin,
> > > >
> > > > Thanks for your testing. I have no more questions about the issue.
> > > >
> > > > Best regards,
> > > > Zheng
> > > >
> > > > > Thanks,
> > > > > Yongqin Liu
> > > > >
> > > > >
> > > > > On Fri, 14 Apr 2023 at 00:46, Zheng Hacker <hackerzheng666@gmail.com> wrote:
> > > > > >
> > > > > > Greg KH <gregkh@linuxfoundation.org> 于2023年4月13日周四 23:56写道：
> > > > > > >
> > > > > > > On Thu, Apr 13, 2023 at 11:35:17PM +0800, Zheng Hacker wrote:
> > > > > > > > Greg KH <gregkh@linuxfoundation.org> 于2023年4月13日周四 20:48写道：
> > > > > > > > >
> > > > > > > > > On Thu, Apr 13, 2023 at 07:12:07PM +0800, Zheng Hacker wrote:
> > > > > > > > > > Yongqin Liu <yongqin.liu@linaro.org> 于2023年4月13日周四 18:55写道：
> > > > > > > > > > >
> > > > > > > > > > > Hi, Zheng
> > > > > > > > > > >
> > > > > > > > > > > On Thu, 13 Apr 2023 at 16:08, Zheng Hacker <hackerzheng666@gmail.com> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > Friendly ping about the bug.
> > > > > > > > > > >
> > > > > > > > > > > Sorry, wasn't aware of this message before,
> > > > > > > > > > >
> > > > > > > > > > > Could you please help share the instructions to reproduce the problem
> > > > > > > > > > > this change fixes?
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Hi Yongqin,
> > > > > > > > > >
> > > > > > > > > > Thanks for your reply. This bug is found by static analysis. There is no PoC.
> > > > > > > > > >
> > > > > > > > > > >From my personal experience, triggering race condition bugs stably in
> > > > > > > > > > the kernel needs some tricks.
> > > > > > > > > > For example, you can insert some sleep-time code to slow down the
> > > > > > > > > > thread until the related object is freed.
> > > > > > > > > > Besides, you can use gdb to control the time window. Also, there are
> > > > > > > > > > some other tricks as [1] said.
> > > > > > > > > >
> > > > > > > > > > As for the reproduction, this attack vector requires that the attacker
> > > > > > > > > > can physically access the device.
> > > > > > > > > > When he/she unplugs the usb, the remove function is triggered, and if
> > > > > > > > > > the set callback is invoked, there might be a race condition.
> > > > > > > > >
> > > > > > > > > How does the removal of the USB device trigger a platform device
> > > > > > > > > removal?
> > > > > > > >
> > > > > > > > Sorry I made a mistake. The USB device usually calls disconnect
> > > > > > > > callback when it's unpluged.
> > > > > > >
> > > > > > > Yes, but you are changing the platform device disconnect, not the USB
> > > > > > > device disconnect.
> > > > > > >
> > > > > > > > What I want to express here is When the driver-related device(here
> > > > > > > > it's USB GPIO Hub) was removed, the remove function is triggered.
> > > > > > >
> > > > > > > And is this a patform device on a USB device?  If so, that's a bigger
> > > > > > > problem that we need to fix as that is not allowed.
> > > > > >
> > > > > > No this is not a platform  device on a USB device.
> > > > > >
> > > > > > >
> > > > > > > But in looking at the code, it does not seem to be that at all, this is
> > > > > > > just a "normal" platform device.  So how can it ever be removed from the
> > > > > > > system?  (and no, unloading the driver doesn't count, that can never
> > > > > > > happen on a normal machine.)
> > > > > > >
> > > > > >
> > > > > > Yes, I finally figured out your meaning. I know it's hard to unplug
> > > > > > the platform device
> > > > > > directly. All I want to express is that it's a theoretical method
> > > > > > except  rmmod. I think it's better to fix the bug. But if the
> > > > > > developers think it's practically impossible, I think there's no need
> > > > > > to take further action.
> > > > > >
> > > > > > Sorry for wasting your time and thanks for your explanation.
> > > > > >
> > > > > > Best regards,
> > > > > > Zheng
> > > > > >
> > > > > > > thanks,
> > > > > > >
> > > > > > > greg k-h
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Best Regards,
> > > > > Yongqin Liu
> > > > > ---------------------------------------------------------------
> > > > > #mailing list
> > > > > linaro-android@lists.linaro.org
> > > > > http://lists.linaro.org/mailman/listinfo/linaro-android
> > >
> > >
> > >
> > > --
> > > Best Regards,
> > > Yongqin Liu
> > > ---------------------------------------------------------------
> > > #mailing list
> > > linaro-android@lists.linaro.org
> > > http://lists.linaro.org/mailman/listinfo/linaro-android
>
>
>
> --
> Best Regards,
> Yongqin Liu
> ---------------------------------------------------------------
> #mailing list
> linaro-android@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-android