usb: dwc3: of-simple: fix use-after-free on remove

["Rafael J. Wysocki" <rafael@kernel.org>]

On Thu, Jun 21, 2018 at 12:32 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Jun 20, 2018 at 5:46 PM, Johan Hovold <johan@kernel.org> wrote:
>> On Wed, Jun 20, 2018 at 02:54:10PM +0200, Rafael J. Wysocki wrote:
>>> On Wednesday, June 20, 2018 2:23:46 PM CEST Johan Hovold wrote:
>>> > On Wed, Jun 20, 2018 at 02:16:59AM -0700, Tony Lindgren wrote:
>>> > > Hi,
>>> > >
>>> > > Adding Rafael and linux-pm to Cc as well.
>>> > >
>>> > > * Felipe Balbi <balbi@kernel.org> [180619 01:23]:
>>> > > > This is a direct consequence of not paying attention to the order of
>>> > > > things. If driver were to assume that pm_domain->activate() would do the
>>> > > > right thing for the device -- meaning that probe would run with an
>>> > > > active device --, then we wouldn't need that pm_runtime_get() call on
>>> > > > probe at all. Rather we would follow the sequence:
>>> > > >
>>> > > >         pm_runtime_forbid()
>>> > > >         pm_runtime_set_active()
>>> > > >         pm_runtime_enable()
>>> > > >
>>> > > >         /* do your probe routine */
>>> > > >
>>> > > >         pm_runtime_put_noidle()
>>> > > >
>>> > > > Then you remove you would need to call pm_runtime_get_noresume() to
>>> > > > balance out the pm_runtime_put_noidle() there.
>>> >
>>> > > > (If you need to know why the pm_runtime_put_noidle(), remember that
>>> > > > pm_runtime_set_active() increments the usage counter, so
>>> > > > pm_runtime_put_noidle is basically allowing pm_runtime to happen as soon
>>> > > > as userspace writes "auto" to /sys/..../power/control)
>>> >
>>> > That's not correct; pm_runtime_set_active() only increments the usage
>>> > counter of a parent (under some circumstances), so unless you have bus
>>> > code incrementing the usage counter before probe, the above
>>> > pm_runtime_put_noidle() would actually introduce an imbalance.
>>>
>>> No, it wouldn't.  It balances the incrementation in pm_runtime_forbid().
>>
>> Right, but even if you take the whole sequence, which included
>> pm_runtime_forbid(), consider what happens when pm_runtime_allow() is
>> later called through sysfs (see below).
>>
>>> > And note that that's also the case even if you meant to say that
>>> > *pm_runtime_forbid()* increments the usage counter (which it does).
>>>
>>> Why is it?
>>>
>>> Surely, after
>>>
>>> pm_runtime_forbid(dev);
>>> pm_runtime_put_noidle(dev);
>>>
>>> the runtime PM usage counter of dev will be the same as before, won't it?
>>
>> Sure, but the imbalance, or rather inconsistent state, has already been
>> introduced.
>>
>> Consider the following sequence of events:
>>
>>                                         usage count
>>                                         0
>> probe()
>>         pm_runtime_forbid()             1
>>         pm_runtime_set_active()
>>         pm_runtime_enable()
>>         pm_runtime_put_noidle()         0
>>
>> Here nothing is preventing the device from runtime suspending, despite
>> runtime PM being forbidden. In fact, it will typically be suspended due
>> to the pm_request_idle() in driver_probe_device(). If later we have:
>>
>> echo auto > power/control
>>         pm_runtime_allow()              -1
>
> OK, you have a point.
>
> After calling pm_runtime_forbid() the driver should allow user space
> to unblock runtime PM for the device - or call pm_runtime_allow()
> itself.

The confusion regarding the pm_runtime_put_noidle() at the end may
come from the special requirement of the PCI bus type as per the
comment in local_pci_probe().
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html