答复: [PATCH] USB:Fix kernel NULL pointer when unbind UHCI form vfio-pci - Weitao Wang(BJ-RD)

From: "Weitao Wang(BJ-RD)" <WeitaoWang@zhaoxin.com>
To: Greg KH <gregkh@linuxfoundation.org>,
	WeitaoWang-oc <WeitaoWang-oc@zhaoxin.com>
Cc: "mathias.nyman@linux.intel.com" <mathias.nyman@linux.intel.com>,
	"ulf.hansson@linaro.org" <ulf.hansson@linaro.org>,
	"vkoul@kernel.org" <vkoul@kernel.org>,
	"hslester96@gmail.com" <hslester96@gmail.com>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Carsten_Schmid@mentor.com" <Carsten_Schmid@mentor.com>,
	"efremov@linux.com" <efremov@linux.com>,
	"Tony W. Wang(XA-RD)" <TonyWWang@zhaoxin.com>,
	"Cobe Chen(BJ-RD)" <CobeChen@zhaoxin.com>,
	"Tim Guo(BJ-RD)" <TimGuo@zhaoxin.com>,
	"wwt8723@163.com" <wwt8723@163.com>
Subject: 答复: [PATCH] USB:Fix kernel NULL pointer when unbind UHCI form vfio-pci
Date: Thu, 23 Jul 2020 02:58:54 +0000	[thread overview]
Message-ID: <e09986ccb3de4b219d1768e24dd89aa8@zhaoxin.com> (raw)
In-Reply-To: <20200722124414.GA3153105@kroah.com>

On Wed, Jul 22, 2020 at 02:44:14PM +0200, Greg KH wrote:
> On Wed, Jul 22, 2020 at 07:57:48PM +0800, WeitaoWangoc wrote:
> > This bug is found in Zhaoxin platform, but it's a commom code bug.
> > Fail sequence:
> > step1: Unbind UHCI controller from native driver;
> > step2: Bind UHCI controller to vfio-pci, which will put UHCI controller in one
> vfio
> >        group's device list and set UHCI's dev->driver_data to struct
> vfio-pci(for UHCI)
>
> Hah, that works?  How do you do that properly?  What code does that?

Drivers/vfio/vfio.c
The function vfio_group_create_device will set UHCI dev->driver_data
to vfio-device struct.

> > step3: Unbind EHCI controller from native driver, will try to tell UHCI native
> driver
> >        that "I'm removed by set companion_hcd->self.hs_companion to
> NULL. However,
> >        companion_hcd get from UHCI's dev->driver_data that has modified
> by vfio-pci
> >        already.So, the vfio-pci structure will be damaged!
>
> Because you are messing around with bind/unbind "by hand", which is
> never guaranteed to work properly.
>
> It's amazing any of that works at all...

If adjust the sequence of UHCI/EHCI unbind form native driver, that
can avoid Null Pointer dereference. Eg. Step3-->stet4-->step1-->step2.
Our experiments prove that this sequence is indeed good as expected.
However, some application software such as virt-manager/qemu assign
/EHCI to guest OS has the same bind/unbind sequence as test “by hand”.

> 4.19.65-2020051917-rainos #1
>
> 4.19 is a bit old, what about 5.7?

5.7.7 has the same issue.

> > +#define PCI_DEV_DRV_FLAG       2
>
> Why is the USB core allowed to touch a private PCI structure field?
>
> I do not think this is allowed.
>
> And why is this a USB host controller-specific issue, shouldn't this
> type of thing be fixed in the PCI core?

When ehci hcd_driver bind or unbind to ehci, it will touch/modify UHCI usb_hcd
that get from UHCI's dev->driver_data. However, UHCI maybe bind to a
another driver(vfio-pci) and it's driver_data will be rewritten. what we
should do is to prevent ehci_hcd to modify UHCI's dev->driver_data when UHCI
has already bound to vfio-pci.

Thanks
weitaowang

保密声明：
本邮件含有保密或专有信息，仅供指定收件人使用。严禁对本邮件或其内容做任何未经授权的查阅、使用、复制或转发。
CONFIDENTIAL NOTE:
This email contains confidential or legally privileged information and is for the sole use of its intended recipient. Any unauthorized review, use, copying or forwarding of this email or the content of this email is strictly prohibited.