[<prev] [next>] [day] [month] [year] [list]
Message-ID: <YrFtaZedCOwSyOof@hovoldconsulting.com>
Date: Tue, 21 Jun 2022 09:04:09 +0200
From: Johan Hovold <johan@...nel.org>
To: 智宋 <zhi.song@...edance.com>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org,
linux-kernel@...r.kernel.org
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@...nel.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
Powered by blists - more mailing lists