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

[Will Deacon <will@xxxxxxxxxx>]

On Thu, Oct 03, 2019 at 02:54:21PM +0200, Linus Walleij wrote:
> 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.

No need to apologise -- I posted it during the merge window!

> 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().

Yeah, but I think that's a least consistent with other sysfs entries
(i.e. those created by driver_sysfs_add()) so callers of dev_set_name()
need to be super careful about how they use it. In reality, it's going
to be mostly confined to bus code, but copying the string (as in this
patch) avoids pinctrl being the thing that blows up.

Will