Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 21 Jun 2022 09:04:09 +0200
From:   Johan Hovold <johan@kernel.org>
To:     =?utf-8?B?5pm65a6L?= <zhi.song@bytedance.com>
Cc:     gregkh@linuxfoundation.org, rafael@kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] node: put_device after failing to device_register
Message-ID: <YrFtaZedCOwSyOof@hovoldconsulting.com>
References: <20220615151738.1766206-1-zhi.song@bytedance.com>
 <YrB8bIJj3fvvsCHg@hovoldconsulting.com>
 <1D7A6F9F-9069-4D87-A493-7DB1BDA1EDB4@bytedance.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1D7A6F9F-9069-4D87-A493-7DB1BDA1EDB4@bytedance.com>
Precedence: bulk

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