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

From: Yongqin Liu
Date: Fri Apr 21 2023 - 11:42:42 EST

Next message: Dario Binacchi: "[PATCH 0/7] add displays support for bsh-smm-s2/pro boards"
Previous message: Jason Gunthorpe: "Re: [PATCH] RDMA/irdma: Drop spurious WQ_UNBOUND from alloc_ordered_workqueue() call"
In reply to: Zheng Hacker: "Re: [PATCH] misc: hisi_hikey_usb: Fix use after free bug in hisi_hikey_usb_remove due to race condition"
Next in thread: Zheng Hacker: "Re: [PATCH] misc: hisi_hikey_usb: Fix use after free bug in hisi_hikey_usb_remove due to race condition"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxx> wrote:
>
> Yongqin Liu <yongqin.liu@xxxxxxxxxx> 于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@xxxxxxxxx> wrote:
> > >
> > > Yongqin Liu <yongqin.liu@xxxxxxxxxx> 于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@xxxxxxxxx> wrote:
> > > > >
> > > > > Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 于2023年4月13日周四 23:56写道：
> > > > > >
> > > > > > On Thu, Apr 13, 2023 at 11:35:17PM +0800, Zheng Hacker wrote:
> > > > > > > Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 于2023年4月13日周四 20:48写道：
> > > > > > > >
> > > > > > > > On Thu, Apr 13, 2023 at 07:12:07PM +0800, Zheng Hacker wrote:
> > > > > > > > > Yongqin Liu <yongqin.liu@xxxxxxxxxx> 于2023年4月13日周四 18:55写道：
> > > > > > > > > >
> > > > > > > > > > Hi, Zheng
> > > > > > > > > >
> > > > > > > > > > On Thu, 13 Apr 2023 at 16:08, Zheng Hacker <hackerzheng666@xxxxxxxxx> 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@xxxxxxxxxxxxxxxx
> > > > http://lists.linaro.org/mailman/listinfo/linaro-android
> >
> >
> >
> > --
> > Best Regards,
> > Yongqin Liu
> > ---------------------------------------------------------------
> > #mailing list
> > linaro-android@xxxxxxxxxxxxxxxx
> > http://lists.linaro.org/mailman/listinfo/linaro-android

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

Next message: Dario Binacchi: "[PATCH 0/7] add displays support for bsh-smm-s2/pro boards"
Previous message: Jason Gunthorpe: "Re: [PATCH] RDMA/irdma: Drop spurious WQ_UNBOUND from alloc_ordered_workqueue() call"
In reply to: Zheng Hacker: "Re: [PATCH] misc: hisi_hikey_usb: Fix use after free bug in hisi_hikey_usb_remove due to race condition"
Next in thread: Zheng Hacker: "Re: [PATCH] misc: hisi_hikey_usb: Fix use after free bug in hisi_hikey_usb_remove due to race condition"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]