Re: [RESEND PATCH] pinctrl: devicetree: Avoid taking direct reference to device name string

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Wed, Oct 2, 2019 at 2:42 PM Will Deacon <will@xxxxxxxxxx> wrote:

> When populating the pinctrl mapping table entries for a device, the
> 'dev_name' field for each entry is initialised to point directly at the
> string returned by 'dev_name()' for the device and subsequently used by
> 'create_pinctrl()' when looking up the mappings for the device being
> probed.
>
> This is unreliable in the presence of calls to 'dev_set_name()', which may
> reallocate the device name string leaving the pinctrl mappings with a
> dangling reference. This then leads to a use-after-free every time the
> name is dereferenced by a device probe:
>
>   | BUG: KASAN: invalid-access in strcmp+0x20/0x64
>   | Read of size 1 at addr 13ffffc153494b00 by task modprobe/590
>   | Pointer tag: [13], memory tag: [fe]
>   |
>   | Call trace:
>   |  __kasan_report+0x16c/0x1dc
>   |  kasan_report+0x10/0x18
>   |  check_memory_region
>   |  __hwasan_load1_noabort+0x4c/0x54
>   |  strcmp+0x20/0x64
>   |  create_pinctrl+0x18c/0x7f4
>   |  pinctrl_get+0x90/0x114
>   |  devm_pinctrl_get+0x44/0x98
>   |  pinctrl_bind_pins+0x5c/0x450
>   |  really_probe+0x1c8/0x9a4
>   |  driver_probe_device+0x120/0x1d8
>
> Follow the example of sysfs, and duplicate the device name string before
> stashing it away in the pinctrl mapping entries.
>
> Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
> Reported-by: Elena Petrova <lenaptr@xxxxxxxxxx>
> Tested-by: Elena Petrova <lenaptr@xxxxxxxxxx>
> Signed-off-by: Will Deacon <will@xxxxxxxxxx>

Patch applied, sorry for not getting back to you earlier.

The fact that dev_set_name() is reallocating the name is a bit
scary, it doesn't feel super-stable, but I suppose there is some
particularly good reason for it.

I guess the look-up table still refers to the struct device *
directly so pin control functionality will work, but the pin controller
device name down in /sys/kernel/debug/pinctrl
is going to be bogus, am I right? Like the name given there
will be whatever the name was before the call to dev_set_name().

Yours,
Lnus Walleij