pinctrl: devicetree: Avoid taking direct reference to device name string
Subsystems (2)
PATCH RESEND 1/1 pinctrl: devicetree: Avoid taking direct reference to device name string
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.
--- drivers/pinctrl/devicetree.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)
drivers/pinctrl/devicetree.c +20 -5
@@ -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)
Signature · 1 line
2.23.0.444.g18eeb5a265-goog
Quoted 28 lines · Will Deacon
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
Quoted 7 lines
Follow the example of sysfs, and duplicate the device name string before stashing it away in the pinctrl mapping entries.
Cc Linus Walleij <[email protected]>Reported-by Elena Petrova <[email protected]>Tested-by Elena Petrova <[email protected]>Signed-off-by Will Deacon <[email protected]>
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
Quoted 32 lines · Linus Walleij
Quoted 31 lines · Will Deacon
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/0x1d8Follow the example of sysfs, and duplicate the device name string before stashing it away in the pinctrl mapping entries.
Quoted 6 lines
Quoted 4 lines
Cc Linus Walleij <[email protected]>Reported-by Elena Petrova <[email protected]>Tested-by Elena Petrova <[email protected]>Signed-off-by Will Deacon <[email protected]>Patch applied, sorry for not getting back to you earlier.
No need to apologise -- I posted it during the merge window!
Quoted 9 lines
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 todev_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