linux-kernel - Re: [PATCH v1 1/1] device property: Allow error pointer for fwnode_handle

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 3 Mar 2022 18:13:30 +0200
From:   Sakari Ailus <sakari.ailus@...ux.intel.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
        Daniel Scally <djrscally@...il.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Nuno Sá <nuno.sa@...log.com>
Subject: Re: [PATCH v1 1/1] device property: Allow error pointer for
 fwnode_handle_{get,put}()

Hi Andy,

On Thu, Mar 03, 2022 at 05:06:10PM +0200, Andy Shevchenko wrote:
> Some of the fwnode APIs might return an error pointer instead of NULL
> or valid fwnode handle. The result of such API call may be considered
> optional and hence the test for it is usually done in a form of
> 
> 	fwnode = fwnode_find_reference(...);
> 	if (IS_ERR_OR_NULL(fwnode))
> 		...error handling...
> 
> Nevertheless the resulting fwnode may have bumped reference count and
> hence caller of the above API is obliged to call fwnode_handle_put().
> Since fwnode may be not valid either as NULL or error pointer the check
> has to be performed there. This approach uglifies the code and adds
> a point of making a mistake, i.e. forgetting about error point case.
> 
> To prevent this allow error pointer for fwnode_handle_get() and
> fwnode_handle_put().
> 
> Fixes: 83b34afb6b79 ("device property: Introduce fwnode_find_reference()")
> Reported-by: Nuno Sá <nuno.sa@...log.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> ---
>  drivers/base/property.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 2d70392fc982..df7b8c7ad264 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -776,7 +776,7 @@ EXPORT_SYMBOL_GPL(device_get_named_child_node);
>   */
>  struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode)
>  {
> -	if (!fwnode_has_op(fwnode, get))
> +	if (IS_ERR(fwnode) || !fwnode_has_op(fwnode, get))
>  		return fwnode;
>  
>  	return fwnode_call_ptr_op(fwnode, get);
> @@ -793,6 +793,9 @@ EXPORT_SYMBOL_GPL(fwnode_handle_get);
>   */
>  void fwnode_handle_put(struct fwnode_handle *fwnode)
>  {
> +	if (IS_ERR(fwnode) || !fwnode_has_op(fwnode, put))
> +		return;
> +
>  	fwnode_call_void_op(fwnode, put);
>  }
>  EXPORT_SYMBOL_GPL(fwnode_handle_put);

I guess fwnode_find_reference() is the only fwnode API function returning
errors as pointers? If you changed it returning NULL on error, you'd lose
the error codes.

But I think this is a problem beyond fwnode_handle_{get,put}: fwnode
obtained this way could be passed to any fwnode function and they should
just work correctly with that.

How about moving the check to fwnode_has_op()? That function is responsible
for checking the fwnode is valid and has the op in question.

It also seems the explicit fwnode_has_op() call is redundant in
fwnode_handle_put() as fwnode_call_void_op() already calls fwnode_has_op().

-- 
Kind regards,

Sakari Ailus