[PATCH v3] node: put_device after failing to device_register

[PATCH v3] node: put_device after failing to device_register

[Zhi Song]

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.

Fixes: 08d9dbe72b1f ("node: Link memory nodes to their compute nodes")
Signed-off-by: Zhi Song <zhi.song@bytedance.com>
---
V1 -> V2: Fix up the changelog text correct.
V2 -> V3: Add a fixes tag line specifying the commit where this bug was
introduced.
---
 drivers/base/node.c | 1 +
 1 file changed, 1 insertion(+)

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);
 free:
 	kfree(access_node);
-- 
2.30.2

Re: [PATCH v3] node: put_device after failing to device_register

[Johan Hovold]

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.
> 
> Fixes: 08d9dbe72b1f ("node: Link memory nodes to their compute nodes")
> Signed-off-by: Zhi Song <zhi.song@bytedance.com>
> ---
> V1 -> V2: Fix up the changelog text correct.
> V2 -> V3: Add a fixes tag line specifying the commit where this bug was
> introduced.
> ---
>  drivers/base/node.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> 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().

Johan

Re: [PATCH v3] node: put_device after failing to device_register

[Johan Hovold]

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

Re: [PATCH v3] node: put_device after failing to device_register

[Johan Hovold]

On Tue, Jun 21, 2022 at 06:17:22PM +0800, 智宋 wrote:
> > On Jun 20, 2022, at 21:55, Johan Hovold <johan@kernel.org> wrote:

> > 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().

> dev_set_name() allocates new space to dev->name and assigns the address of space to dev->name if it allocates successfully. But if we fail to allocate space, there isn’t any new space for dev->name. Therefore, there’s no need for calling kfree_const(dev->kobj.name) in dev_set_name()’s error handling.

Note that your mails are being rejected by the mailing list since they
include HTML. Can you see if you can fix your mail client to send as
text instead?

That may fix lack line breaks too (break lines at 72 columns or so).
 
> If we fail to do device_register(dev), we just need to put_device(dev) which will do free access_node, kobj.name and other cleanup.
> 
> Maybe the code is: 
> 
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -144,21 +144,19 @@ 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))
> -               goto free;
> +       if (dev_set_name(dev, "access%u", access)) {
> +               kfree(access_node);
> +               return NULL;
> +       }
>  
> -       if (device_register(dev))
> -               goto free_name;
> +       if (device_register(dev)) {
> +               put_device(dev);
> +               return NULL;
> +       }
>  
>         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);
> -       return NULL;
>  }

The above fix looks correct now.

> Thanks for your patience. :)

You're welcome. Thanks for finding and fixing the bug.

Johan