linux-kernel - Re: [PATCH v4 1/1] device property: Allow error pointer to be passed to fwnode APIs

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]

Message-Id: <20220314195138.20036-1-michael@walle.cc>
Date:   Mon, 14 Mar 2022 20:51:37 +0100
From:   Michael Walle <michael@...le.cc>
To:     andriy.shevchenko@...ux.intel.com
Cc:     djrscally@...il.com, gregkh@...uxfoundation.org,
        heikki.krogerus@...ux.intel.com, lenb@...nel.org,
        linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
        nuno.sa@...log.com, rafael@...nel.org,
        sakari.ailus@...ux.intel.com, Russell King <linux@...linux.org.uk>,
        Horatiu Vultur <horatiu.vultur@...rochip.com>,
        Michael Walle <michael@...le.cc>
Subject: Re: [PATCH v4 1/1] device property: Allow error pointer to be passed to fwnode APIs

Hi Andy,

> 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(fwnode))
> 		...error handling...
> 
> Nevertheless the resulting fwnode may have bumped the 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 an error pointer to be passed to the fwnode APIs.
> 
> 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>
> Tested-by: Nuno Sá <nuno.sa@...log.com>
> Reviewed-by: Sakari Ailus <sakari.ailus@...ux.intel.com>
> Acked-by: Nuno Sá <nuno.sa@...log.com>

This breaks SFP/phylink (using the lan966x switch) on my board. See below
for more details.

[..]

> @@ -480,15 +485,16 @@ int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
>  {
>  	int ret;
>  
> +	if (IS_ERR_OR_NULL(fwnode))
> +		return -ENOENT;
> +
>  	ret = fwnode_call_int_op(fwnode, get_reference_args, prop, nargs_prop,
>  				 nargs, index, args);
> +	if (ret == 0)

Should this be "if (ret == 0 || IS_ERR_OR_NULL(fwnode->secondary))" ?

> +		return ret;
>  
> -	if (ret < 0 && !IS_ERR_OR_NULL(fwnode) &&
> -	    !IS_ERR_OR_NULL(fwnode->secondary))
> -		ret = fwnode_call_int_op(fwnode->secondary, get_reference_args,
> -					 prop, nargs_prop, nargs, index, args);
> -
> -	return ret;
> +	return fwnode_call_int_op(fwnode->secondary, get_reference_args, prop, nargs_prop,
> +				  nargs, index, args);
>  }
>  EXPORT_SYMBOL_GPL(fwnode_property_get_reference_args);

What happens before this patch is that sfp_bus_find_fwnode() will call
fwnode_property_get_reference_args() and the first calls return -ENOENT
which sfp_bus_find_fwnode() will handle in a special way. After your
patch, -EINVAL is returned, because fwnode_call_int_op() on
fwnode->secondary is always called regardless of the return value of
the original fwnode.

-michael