misc: hisi_hikey_usb: Fix use after free bug in hisi_hikey_usb_remove due to race condition
Message ID | 20230312145305.1908607-1-zyytlz.wz@163.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:5915:0:0:0:0:0 with SMTP id v21csp759500wrd; Sun, 12 Mar 2023 08:18:26 -0700 (PDT) X-Google-Smtp-Source: AK7set+tRLoOECO0tZvYhuxNhQv2ZP9e4fq5UtbSiOM0yDBZqNOFsAKyY7jA2kgjEeHBHLOj9oGs X-Received: by 2002:aa7:9901:0:b0:624:9bdc:f255 with SMTP id z1-20020aa79901000000b006249bdcf255mr325522pff.21.1678634306622; Sun, 12 Mar 2023 08:18:26 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1678634306; cv=none; d=google.com; s=arc-20160816; b=Gy1BOGdL9AOyl9/Dy/b9MCpiN3eq4oYXoO92YlV67Xtu+HJ5+4g6PmU5+x91ATu8pV 3uzmguZ/Yelt75BRJXauxRTzldnQItz8t/Ev/7RF4CAL6v5ONQQ0kv7MGTTPSyA9QPnY BBbjUgI1BUz7A1JXo0eKBJlcAL2bLSurL+Ewgh/9OO5FHSuIt8bkt2U1JISfhA+Mg9/K YV2FCud/BT46QE0K/g8Uk/tdsPcRw2ZNccXTq3BtqY60hAQX0HLytAgDtg4u/orThUUt 2T64UQkJb0oioJ9kd4K13BaNjCIUPA6Mvn8LE0BbhN/5M/8i4GloMWKrnT97Fw8d29t+ CGVA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=SLbILnb3t/Z+sQSGFQnXNsUhmpJHie1CiaHgAtT157Q=; b=JlFfrGS0euSzwrI4VOZzYnA6oTRKNN/00P9K0VVhyywAYUfM5w51j2AdG6yXhokgsP Wg8fM5R4ZDHMkm8AhwEE0wGOBhm7eQ7OTVBJuYvUl/QP0628IhT73/LL4DTsd6l/BFC+ /UBuaUs61BAFNBR6+gBoM4XB11I14fRvh/O80nZGBmcZ54ZAABly6c0kO0lHsdFew3IL tTsw+HdZal64R7jRlDDqPXXlb/8BxScOd4+rjMU3hLc4yJ/iLM8lNoiTTM5vWmSSG8oG 2vrPDqAjqin49vyR/IwWbWE3Pojl1zmmlIUFxtrhGmnhCZnyArouaRtXwdNIivM0DTol CdlA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@163.com header.s=s110527 header.b=MP8woXRV; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=163.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id g11-20020a63564b000000b005031c584be5si4348136pgm.624.2023.03.12.08.18.12; Sun, 12 Mar 2023 08:18:26 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@163.com header.s=s110527 header.b=MP8woXRV; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=163.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229800AbjCLOxj (ORCPT <rfc822;realc9580@gmail.com> + 99 others); Sun, 12 Mar 2023 10:53:39 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56858 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229711AbjCLOxg (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sun, 12 Mar 2023 10:53:36 -0400 Received: from m12.mail.163.com (m12.mail.163.com [220.181.12.215]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 64D112201D for <linux-kernel@vger.kernel.org>; Sun, 12 Mar 2023 07:53:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=163.com; s=s110527; h=From:Subject:Date:Message-Id:MIME-Version; bh=SLbIL nb3t/Z+sQSGFQnXNsUhmpJHie1CiaHgAtT157Q=; b=MP8woXRV8C3Y3ajlQdNkK 6zNpXRDqS3MTkDVCE6JPak9PwLXzPvGSjLtfAnHyFGol23g+ZiIFZgiF/XeGRZ00 g64YVFIZxXZQ8AqreF6TQ5mtsvG0X3iRl1rpz8+2eT4+D2mMxJc3lvhXY1WUySLK wouvhMssNUuVMSWnEttnE8= Received: from leanderwang-LC2.localdomain (unknown [111.206.145.21]) by zwqz-smtp-mta-g4-2 (Coremail) with SMTP id _____wBH5bVS5w1ksCV3DA--.7216S2; Sun, 12 Mar 2023 22:53:06 +0800 (CST) From: Zheng Wang <zyytlz.wz@163.com> To: jstultz@google.com Cc: arnd@arndb.de, gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, hackerzheng666@gmail.com, 1395428693sheep@gmail.com, alex000young@gmail.com, Zheng Wang <zyytlz.wz@163.com> Subject: [PATCH] misc: hisi_hikey_usb: Fix use after free bug in hisi_hikey_usb_remove due to race condition Date: Sun, 12 Mar 2023 22:53:05 +0800 Message-Id: <20230312145305.1908607-1-zyytlz.wz@163.com> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-CM-TRANSID: _____wBH5bVS5w1ksCV3DA--.7216S2 X-Coremail-Antispam: 1Uf129KBjvJXoW7KFWkGw4UuF1xtrWfZF4xJFb_yoW8GFyxpr WxGw1UWrWUtFW7tw47JFsYqFyYga1xJa4UZw4UCw1fZ3s8Aw48XFy8JF4Ygr4xJFZFvFWr ZF4xWFyxua48GrUanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDUYxBIdaVFxhVjvjDU0xZFpf9x0ziaiiDUUUUU= X-Originating-IP: [111.206.145.21] X-CM-SenderInfo: h2113zf2oz6qqrwthudrp/xtbBzhkwU2I0Xm-18wAAsV X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,SPF_HELO_NONE, SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1760175646644474462?= X-GMAIL-MSGID: =?utf-8?q?1760175646644474462?= |
Series |
misc: hisi_hikey_usb: Fix use after free bug in hisi_hikey_usb_remove due to race condition
|
|
Commit Message
Zheng Wang
March 12, 2023, 2:53 p.m. UTC
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(+)
Comments
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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);