Re: Use after free with pinctrl_enable() and devm_pinctrl_register_and_init()

[Tony Lindgren]

* Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> [180321 15:44]:
> Hi Linus, Tony,
> 
> If claiming hogs failed, pinctrl_enable() frees the pctldev, and
> destroys its mutex.

Hmm so is something also left dangling at this point?

> However, if the pin controller is registered using
> devm_pinctrl_register_and_init(), device resource management will call
> pinctrl_unregister() later.  This will access the destroyed pctldev,
> which may crash the system.

Can we call pinctrl_unregister() earlier and just set
pctldev to NULL..

> With poisoning enabled (CONFIG_DEBUG_SLAB=y), the crash is imminent:
> 
>     sh-pfc e6050000.pin-controller: function 'foo' not supported
>     sh-pfc e6050000.pin-controller: invalid function a in map table
>     sh-pfc e6050000.pin-controller: error claiming hogs: -22
>     sh-pfc e6050000.pin-controller: could not claim hogs: -22
>     Unhandled fault: alignment exception (0x001) at 0x6b6b6c2f
>     pgd = 581794e0
>     [6b6b6c2f] *pgd=00000000
>     Internal error: : 1 [#1] SMP ARM
>     Modules linked in:
>     CPU: 0 PID: 1 Comm: swapper/0 Not tainted
> 4.16.0-rc5-kzm9g-00484-gb3469948b11d02a0-dirty #1043
>     Hardware name: Generic SH73A0 (Flattened Device Tree)
>     PC is at __lock_acquire+0xd8/0x1bf0
>     LR is at lock_acquire+0x98/0xb8
>     pc : [<c016fc84>]    lr : [<c0171f80>]    psr: 20000093
>     sp : df441be8  ip : df441c80  fp : 00000000
>     r10: 00000000  r9 : 00000001  r8 : 00000000
>     r7 : 00000000  r6 : c1084170  r5 : df5586e8  r4 : df43e040
>     r3 : 6b6b6c2f  r2 : 6b6b6b6b  r1 : 00000000  r0 : 6b6b6b6b
>     Flags: nzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
>     Control: 10c5387d  Table: 4000404a  DAC: 00000051
>     Process swapper/0 (pid: 1, stack limit = 0x2557051f)
>     Stack: (0xdf441be8 to 0xdf442000)
>     ...
>     [<c016fc84>] (__lock_acquire) from [<c0171f80>] (lock_acquire+0x98/0xb8)
>     [<c0171f80>] (lock_acquire) from [<c0598888>] (__mutex_lock+0x7c/0x9ec)
>     [<c0598888>] (__mutex_lock) from [<c0599210>] (mutex_lock_nested+0x18/0x20)
>     [<c0599210>] (mutex_lock_nested) from [<c034cd18>]
> (pinctrl_unregister+0x48/0x158)
>     [<c034cd18>] (pinctrl_unregister) from [<c03b3f30>]
> (release_nodes+0x218/0x25c)
>     [<c03b3f30>] (release_nodes) from [<c03b08ac>]
> (driver_probe_device+0x200/0x318)
>     [<c03b08ac>] (driver_probe_device) from [<c03aedd8>]
> (bus_for_each_drv+0x90/0xb8)
> 
> My first idea was to add a !devres_find(pctldev->dev, devm_pinctrl_dev_release,
> NULL, NULL) check to the error path of pinctrl_enable(), which seems
> to work(TM).
> 
> However, that's still not 100% correct and sufficient:
>   - pinctrl_unregister() will do some cleanup (pinctrldev_list and debugfs) that
>     should not be done,

.. or maybe we could add some state flag to prevent this?

Something like the following pseudo code with super long
names maybe :)

enum pinctrl_controller_state {
     PINCTRL_STATE_NONE,
     PINCTRL_STATE_REGISTERED,
     PINCTRL_STATE_HOGS_CLAIMED,
     PINCTRL_STATE_INITIALIZED,
     PINCTRL_STATE_ENABLED,
};

>   - When using pinctrl_register() or devm_pinctrl_register(), and
>     pinctrl_enable() fails, all other cleanup done in pinctrl_unregister()
>     never happens.

Maybe this too would get sorted out if we could call
pinctrl_unregister() safely at any point of a failed
controller init?

Regards,

Tony