Messages in this thread | | | Date | Tue, 21 Jun 2022 09:04:09 +0200 | From | Johan Hovold <> | Subject | Re: [PATCH v3] node: put_device after failing to device_register |
| |
On Tue, Jun 21, 2022 at 01:28:33AM +0800, 智宋 wrote: > > On Jun 20, 2022, at 21:55, Johan Hovold <johan@kernel.org> wrote: > > On Wed, Jun 15, 2022 at 11:17:38PM +0800, Zhi Song wrote: > >> device_register() is used to register a device with the system. > >> We need to call put_device() to give up the reference initialized > >> in device_register() when it returns an error and this will clean > >> up correctly.
> >> diff --git a/drivers/base/node.c b/drivers/base/node.c > >> index 0ac6376ef7a1..88a3337c546e 100644 > >> --- a/drivers/base/node.c > >> +++ b/drivers/base/node.c > >> @@ -154,6 +154,7 @@ static struct node_access_nodes *node_init_node_access(struct node *node, > >> list_add_tail(&access_node->list_node, &node->access_list); > >> return access_node; > >> free_name: > >> + put_device(dev); > >> kfree_const(dev->kobj.name); > > > > That's a pretty obvious use-after-free you just added here. You can't > > access dev after you've just freed it. > > > > The name is freed along with the rest of the struct device so you need > > to remove the second explicit free. And you should rename the label too. > > > >> free: > >> kfree(access_node); > > > > But here's another use after free... The put_device() call you added > > will have freed access_node by calling node_access_release().
> put_device will free kobj.name at kobject_put => kref_put => > kobject_release => kobject_cleanup => kfree_const(name) > and free access_node at kobject_put => kref_put => kobject_release > => kobject_cleanup => t->release(kobj). (The value of t->release is > device_release, it will invoke dev->release that is node_access_release) > > If name is not set, kfree_const (name) won’t be invoked in kobject_cleanup.
Right.
> If we fail to invoke dev_set_name or device_register, we just need > to invoke put_device(dev) which will process free name, free access_node > and other jobs. > > Therefore, the proper code would be this: > --- a/drivers/base/node.c > +++ b/drivers/base/node.c > @@ -144,20 +144,14 @@ static struct node_access_nodes *node_init_node_access(struct node *node, > dev->parent = &node->dev; > dev->release = node_access_release; > dev->groups = node_access_node_groups; > - if (dev_set_name(dev, "access%u", access)) > + if (dev_set_name(dev, "access%u", access) && device_register(dev)) > goto free; > > - if (device_register(dev)) > - goto free_name; > - > pm_runtime_no_callbacks(dev); > list_add_tail(&access_node->list_node, &node->access_list); > return access_node; > -free_name: > - put_device(dev); > - kfree_const(dev->kobj.name); > free: > - kfree(access_node); > + put_device(dev); > return NULL; > } > > The only put_device is enough. Is it correct?
I'm afraid not. You can only call put_device() after the struct device has been initialised by device_initialize(), which is done in device_register(), so you need to restructure the error handling somewhat.
Johan
| |