| Message ID | 20191002124206.22928-1-will@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] pinctrl: devicetree: Avoid taking direct reference to device name string | expand |
On Wed, Oct 2, 2019 at 2:42 PM Will Deacon <will@kernel.org> 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@linaro.org> > Reported-by: Elena Petrova <lenaptr@google.com> > Tested-by: Elena Petrova <lenaptr@google.com> > Signed-off-by: Will Deacon <will@kernel.org> 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
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@kernel.org> 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@linaro.org> > > Reported-by: Elena Petrova <lenaptr@google.com> > > Tested-by: Elena Petrova <lenaptr@google.com> > > Signed-off-by: Will Deacon <will@kernel.org> > > 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
diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c index 88ddbb2e30de..9721ad30c541 100644 --- a/drivers/pinctrl/devicetree.c +++ b/drivers/pinctrl/devicetree.c @@ -29,6 +29,13 @@ struct pinctrl_dt_map { static void dt_free_map(struct pinctrl_dev *pctldev, struct pinctrl_map *map, unsigned num_maps) { + int i; + + for (i = 0; i < num_maps; ++i) { + kfree_const(map[i].dev_name); + map[i].dev_name = NULL; + } + if (pctldev) { const struct pinctrl_ops *ops = pctldev->desc->pctlops; if (ops->dt_free_map) @@ -63,7 +70,13 @@ static int dt_remember_or_free_map(struct pinctrl *p, const char *statename, /* Initialize common mapping table entry fields */ for (i = 0; i < num_maps; i++) { - map[i].dev_name = dev_name(p->dev); + const char *devname; + + devname = kstrdup_const(dev_name(p->dev), GFP_KERNEL); + if (!devname) + goto err_free_map; + + map[i].dev_name = devname; map[i].name = statename; if (pctldev) map[i].ctrl_dev_name = dev_name(pctldev->dev); @@ -71,10 +84,8 @@ static int dt_remember_or_free_map(struct pinctrl *p, const char *statename, /* Remember the converted mapping table entries */ dt_map = kzalloc(sizeof(*dt_map), GFP_KERNEL); - if (!dt_map) { - dt_free_map(pctldev, map, num_maps); - return -ENOMEM; - } + if (!dt_map) + goto err_free_map; dt_map->pctldev = pctldev; dt_map->map = map; @@ -82,6 +93,10 @@ static int dt_remember_or_free_map(struct pinctrl *p, const char *statename, list_add_tail(&dt_map->node, &p->dt_maps); return pinctrl_register_map(map, num_maps, false); + +err_free_map: + dt_free_map(pctldev, map, num_maps); + return -ENOMEM; } struct pinctrl_dev *of_pinctrl_get(struct device_node *np)