| Message ID | 20201021184351.17328-1-khalid.elmously@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,X,1/1] pinctrl: devicetree: Avoid taking direct reference to device name string | expand |
On 21.10.20 20:43, Khalid Elmously wrote: > From: Will Deacon <will@kernel.org> > > CVE-2020-0427 > > 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> > Link: https://lore.kernel.org/r/20191002124206.22928-1-will@kernel.org > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > (backported from commit be4c60b563edee3712d392aaeb0943a768df7023) > [ kmously: Extra dev_err() in dt_remember_or_free_map() required > manual merging ] > Signed-off-by: Khalid Elmously <khalid.elmously@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- > drivers/pinctrl/devicetree.c | 22 +++++++++++++++++++--- > 1 file changed, 19 insertions(+), 3 deletions(-) > > diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c > index fe04e748dfe4b..74eebcbd4e057 100644 > --- a/drivers/pinctrl/devicetree.c > +++ b/drivers/pinctrl/devicetree.c > @@ -40,6 +40,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; > ops->dt_free_map(pctldev, map, num_maps); > @@ -73,7 +80,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); > @@ -83,8 +96,7 @@ static int dt_remember_or_free_map(struct pinctrl *p, const char *statename, > dt_map = kzalloc(sizeof(*dt_map), GFP_KERNEL); > if (!dt_map) { > dev_err(p->dev, "failed to alloc struct pinctrl_dt_map\n"); > - dt_free_map(pctldev, map, num_maps); > - return -ENOMEM; > + goto err_free_map; > } > > dt_map->pctldev = pctldev; > @@ -93,6 +105,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) >
On Wed, Oct 21, 2020 at 02:43:51PM -0400, Khalid Elmously wrote: > From: Will Deacon <will@kernel.org> > > CVE-2020-0427 > > 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> > Link: https://lore.kernel.org/r/20191002124206.22928-1-will@kernel.org > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > (backported from commit be4c60b563edee3712d392aaeb0943a768df7023) > [ kmously: Extra dev_err() in dt_remember_or_free_map() required > manual merging ] > Signed-off-by: Khalid Elmously <khalid.elmously@canonical.com> Acked-by: Andrea Righi <andrea.righi@canonical.com>
Applied to Xenial/master-next Thanks, Ian On 2020-10-21 14:43:51 , Khalid Elmously wrote: > From: Will Deacon <will@kernel.org> > > CVE-2020-0427 > > 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> > Link: https://lore.kernel.org/r/20191002124206.22928-1-will@kernel.org > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > (backported from commit be4c60b563edee3712d392aaeb0943a768df7023) > [ kmously: Extra dev_err() in dt_remember_or_free_map() required > manual merging ] > Signed-off-by: Khalid Elmously <khalid.elmously@canonical.com> > --- > drivers/pinctrl/devicetree.c | 22 +++++++++++++++++++--- > 1 file changed, 19 insertions(+), 3 deletions(-) > > diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c > index fe04e748dfe4b..74eebcbd4e057 100644 > --- a/drivers/pinctrl/devicetree.c > +++ b/drivers/pinctrl/devicetree.c > @@ -40,6 +40,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; > ops->dt_free_map(pctldev, map, num_maps); > @@ -73,7 +80,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); > @@ -83,8 +96,7 @@ static int dt_remember_or_free_map(struct pinctrl *p, const char *statename, > dt_map = kzalloc(sizeof(*dt_map), GFP_KERNEL); > if (!dt_map) { > dev_err(p->dev, "failed to alloc struct pinctrl_dt_map\n"); > - dt_free_map(pctldev, map, num_maps); > - return -ENOMEM; > + goto err_free_map; > } > > dt_map->pctldev = pctldev; > @@ -93,6 +105,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) > -- > 2.17.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c index fe04e748dfe4b..74eebcbd4e057 100644 --- a/drivers/pinctrl/devicetree.c +++ b/drivers/pinctrl/devicetree.c @@ -40,6 +40,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; ops->dt_free_map(pctldev, map, num_maps); @@ -73,7 +80,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); @@ -83,8 +96,7 @@ static int dt_remember_or_free_map(struct pinctrl *p, const char *statename, dt_map = kzalloc(sizeof(*dt_map), GFP_KERNEL); if (!dt_map) { dev_err(p->dev, "failed to alloc struct pinctrl_dt_map\n"); - dt_free_map(pctldev, map, num_maps); - return -ENOMEM; + goto err_free_map; } dt_map->pctldev = pctldev; @@ -93,6 +105,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)