| Message ID | 20211005091016.18519-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/1] gpio: mockup: Convert to use software nodes | expand |
On Tue, Oct 05, 2021 at 12:10:16PM +0300, Andy Shevchenko wrote: > The gpio-mockup driver creates a properties that are shared between > platform and GPIO devices. Because of that, the properties may not > be removed at the proper point of time without provoking use-after-free > as shown in the backtrace: > > refcount_t: underflow; use-after-free. > WARNING: CPU: 0 PID: 103 at lib/refcount.c:28 refcount_warn_saturate+0xd1/0x120 > ... > Call Trace: > kobject_put+0xdc/0xf0 > software_node_notify_remove+0xa8/0xc0 > device_del+0x15a/0x3e0 > > That's why the driver has to manage lifetime of the software nodes by itself. > > The problem originates by the old device_add_properties() API, but has been > only revealed after the commit 5aeb05b27f81 ("software node: balance refcount > for managed software nodes"). Hence, it's used as landmark for the backporting. > > Fixes: 5aeb05b27f81 ("software node: balance refcount for managed software nodes") Shouldn't that be: Fixes: bd1e336aa853 ("driver core: platform: Remove platform_device_add_properties()") > Reported-by: Kent Gibson <warthog618@gmail.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Other than that, looks good and works for me. Tested-by: Kent Gibson <warthog618@gmail.com> Cheers, Kent. > --- > drivers/gpio/gpio-mockup.c | 22 +++++++++++++++++++--- > 1 file changed, 19 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c > index 0a9d746a0fe0..8b147b565e92 100644 > --- a/drivers/gpio/gpio-mockup.c > +++ b/drivers/gpio/gpio-mockup.c > @@ -478,8 +478,18 @@ static void gpio_mockup_unregister_pdevs(void) > { > int i; > > - for (i = 0; i < GPIO_MOCKUP_MAX_GC; i++) > - platform_device_unregister(gpio_mockup_pdevs[i]); > + for (i = 0; i < GPIO_MOCKUP_MAX_GC; i++) { > + struct platform_device *pdev; > + struct fwnode_handle *fwnode; > + > + pdev = gpio_mockup_pdevs[i]; > + if (!pdev) > + continue; > + > + fwnode = dev_fwnode(&pdev->dev); > + platform_device_unregister(pdev); > + fwnode_remove_software_node(fwnode); > + } > } > > static __init char **gpio_mockup_make_line_names(const char *label, > @@ -508,6 +518,7 @@ static int __init gpio_mockup_register_chip(int idx) > struct property_entry properties[GPIO_MOCKUP_MAX_PROP]; > struct platform_device_info pdevinfo; > struct platform_device *pdev; > + struct fwnode_handle *fwnode; > char **line_names = NULL; > char chip_label[32]; > int prop = 0, base; > @@ -536,13 +547,18 @@ static int __init gpio_mockup_register_chip(int idx) > "gpio-line-names", line_names, ngpio); > } > > + fwnode = fwnode_create_software_node(properties, NULL); > + if (IS_ERR(fwnode)) > + return PTR_ERR(fwnode); > + > pdevinfo.name = "gpio-mockup"; > pdevinfo.id = idx; > - pdevinfo.properties = properties; > + pdevinfo.fwnode = fwnode; > > pdev = platform_device_register_full(&pdevinfo); > kfree_strarray(line_names, ngpio); > if (IS_ERR(pdev)) { > + fwnode_remove_software_node(fwnode); > pr_err("error registering device"); > return PTR_ERR(pdev); > } > -- > 2.33.0 >
On Tue, Oct 5, 2021 at 12:28 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Tue, Oct 05, 2021 at 12:10:16PM +0300, Andy Shevchenko wrote: > > The gpio-mockup driver creates a properties that are shared between > > platform and GPIO devices. Because of that, the properties may not > > be removed at the proper point of time without provoking use-after-free > > as shown in the backtrace: > > > > refcount_t: underflow; use-after-free. > > WARNING: CPU: 0 PID: 103 at lib/refcount.c:28 refcount_warn_saturate+0xd1/0x120 > > ... > > Call Trace: > > kobject_put+0xdc/0xf0 > > software_node_notify_remove+0xa8/0xc0 > > device_del+0x15a/0x3e0 > > > > That's why the driver has to manage lifetime of the software nodes by itself. > > > > The problem originates by the old device_add_properties() API, but has been > > only revealed after the commit 5aeb05b27f81 ("software node: balance refcount > > for managed software nodes"). Hence, it's used as landmark for the backporting. > > > > Fixes: 5aeb05b27f81 ("software node: balance refcount for managed software nodes") > > Shouldn't that be: > Fixes: bd1e336aa853 ("driver core: platform: Remove platform_device_add_properties()") I think you are right. I thought that it happened during the last rc-week. > > Reported-by: Kent Gibson <warthog618@gmail.com> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Other than that, looks good and works for me. > > Tested-by: Kent Gibson <warthog618@gmail.com> Thanks!
On Tue, Oct 5, 2021 at 11:10 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > The gpio-mockup driver creates a properties that are shared between > platform and GPIO devices. Because of that, the properties may not > be removed at the proper point of time without provoking use-after-free > as shown in the backtrace: > > refcount_t: underflow; use-after-free. > WARNING: CPU: 0 PID: 103 at lib/refcount.c:28 refcount_warn_saturate+0xd1/0x120 > ... > Call Trace: > kobject_put+0xdc/0xf0 > software_node_notify_remove+0xa8/0xc0 > device_del+0x15a/0x3e0 > > That's why the driver has to manage lifetime of the software nodes by itself. > > The problem originates by the old device_add_properties() API, but has been > only revealed after the commit 5aeb05b27f81 ("software node: balance refcount > for managed software nodes"). Hence, it's used as landmark for the backporting. > > Fixes: 5aeb05b27f81 ("software node: balance refcount for managed software nodes") > Reported-by: Kent Gibson <warthog618@gmail.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/gpio/gpio-mockup.c | 22 +++++++++++++++++++--- > 1 file changed, 19 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c > index 0a9d746a0fe0..8b147b565e92 100644 > --- a/drivers/gpio/gpio-mockup.c > +++ b/drivers/gpio/gpio-mockup.c > @@ -478,8 +478,18 @@ static void gpio_mockup_unregister_pdevs(void) > { > int i; > > - for (i = 0; i < GPIO_MOCKUP_MAX_GC; i++) > - platform_device_unregister(gpio_mockup_pdevs[i]); > + for (i = 0; i < GPIO_MOCKUP_MAX_GC; i++) { > + struct platform_device *pdev; > + struct fwnode_handle *fwnode; > + > + pdev = gpio_mockup_pdevs[i]; > + if (!pdev) > + continue; > + > + fwnode = dev_fwnode(&pdev->dev); > + platform_device_unregister(pdev); > + fwnode_remove_software_node(fwnode); > + } > } > > static __init char **gpio_mockup_make_line_names(const char *label, > @@ -508,6 +518,7 @@ static int __init gpio_mockup_register_chip(int idx) > struct property_entry properties[GPIO_MOCKUP_MAX_PROP]; > struct platform_device_info pdevinfo; > struct platform_device *pdev; > + struct fwnode_handle *fwnode; > char **line_names = NULL; > char chip_label[32]; > int prop = 0, base; > @@ -536,13 +547,18 @@ static int __init gpio_mockup_register_chip(int idx) > "gpio-line-names", line_names, ngpio); > } > > + fwnode = fwnode_create_software_node(properties, NULL); > + if (IS_ERR(fwnode)) > + return PTR_ERR(fwnode); > + > pdevinfo.name = "gpio-mockup"; > pdevinfo.id = idx; > - pdevinfo.properties = properties; > + pdevinfo.fwnode = fwnode; > > pdev = platform_device_register_full(&pdevinfo); > kfree_strarray(line_names, ngpio); > if (IS_ERR(pdev)) { > + fwnode_remove_software_node(fwnode); > pr_err("error registering device"); > return PTR_ERR(pdev); > } > -- > 2.33.0 > I suppose my gpio-sim patch is affected by this issue too. Thanks for fixing that. Bart
On Tue, Oct 5, 2021 at 12:35 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Tue, Oct 5, 2021 at 11:10 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > I suppose my gpio-sim patch is affected by this issue too. Thanks for > fixing that. As far as I remember the code (it was pretty much inherited from gpio-mockup) yes, it's also affected. Btw, I just sent a v2.
diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c index 0a9d746a0fe0..8b147b565e92 100644 --- a/drivers/gpio/gpio-mockup.c +++ b/drivers/gpio/gpio-mockup.c @@ -478,8 +478,18 @@ static void gpio_mockup_unregister_pdevs(void) { int i; - for (i = 0; i < GPIO_MOCKUP_MAX_GC; i++) - platform_device_unregister(gpio_mockup_pdevs[i]); + for (i = 0; i < GPIO_MOCKUP_MAX_GC; i++) { + struct platform_device *pdev; + struct fwnode_handle *fwnode; + + pdev = gpio_mockup_pdevs[i]; + if (!pdev) + continue; + + fwnode = dev_fwnode(&pdev->dev); + platform_device_unregister(pdev); + fwnode_remove_software_node(fwnode); + } } static __init char **gpio_mockup_make_line_names(const char *label, @@ -508,6 +518,7 @@ static int __init gpio_mockup_register_chip(int idx) struct property_entry properties[GPIO_MOCKUP_MAX_PROP]; struct platform_device_info pdevinfo; struct platform_device *pdev; + struct fwnode_handle *fwnode; char **line_names = NULL; char chip_label[32]; int prop = 0, base; @@ -536,13 +547,18 @@ static int __init gpio_mockup_register_chip(int idx) "gpio-line-names", line_names, ngpio); } + fwnode = fwnode_create_software_node(properties, NULL); + if (IS_ERR(fwnode)) + return PTR_ERR(fwnode); + pdevinfo.name = "gpio-mockup"; pdevinfo.id = idx; - pdevinfo.properties = properties; + pdevinfo.fwnode = fwnode; pdev = platform_device_register_full(&pdevinfo); kfree_strarray(line_names, ngpio); if (IS_ERR(pdev)) { + fwnode_remove_software_node(fwnode); pr_err("error registering device"); return PTR_ERR(pdev); }
The gpio-mockup driver creates a properties that are shared between platform and GPIO devices. Because of that, the properties may not be removed at the proper point of time without provoking use-after-free as shown in the backtrace: refcount_t: underflow; use-after-free. WARNING: CPU: 0 PID: 103 at lib/refcount.c:28 refcount_warn_saturate+0xd1/0x120 ... Call Trace: kobject_put+0xdc/0xf0 software_node_notify_remove+0xa8/0xc0 device_del+0x15a/0x3e0 That's why the driver has to manage lifetime of the software nodes by itself. The problem originates by the old device_add_properties() API, but has been only revealed after the commit 5aeb05b27f81 ("software node: balance refcount for managed software nodes"). Hence, it's used as landmark for the backporting. Fixes: 5aeb05b27f81 ("software node: balance refcount for managed software nodes") Reported-by: Kent Gibson <warthog618@gmail.com> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/gpio/gpio-mockup.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)