Re: [PATCH 2/2] device property: fix for a case of use-after-free

[Shevchenko, Andriy]

On Mon, 2016-02-22 at 17:40 +0200, Andy Shevchenko wrote:
> On Mon, 2016-02-22 at 16:50 +0200, Heikki Krogerus wrote:
> > In device_remove_property_set(), if the primary fwnode is
> > of type "pset", it has to be set pointing to NULL before
> > calling set_secondary_fwnode(). Otherwise
> > set_secondary_fwnode() will attempt to set the
> > fwnode->secondary member after the fwnode has been freed.
> > 
> > Reported-by: John Youn <John.Youn@xxxxxxxxxxxx>
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> > ---
> > Âdrivers/base/property.c | 4 +++-
> > Â1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > index a163f2c..ddf2987 100644
> > --- a/drivers/base/property.c
> > +++ b/drivers/base/property.c
> > @@ -820,7 +820,9 @@ void device_remove_property_set(struct device
> > *dev)
> > Â	Â* the pset. If there is no real firmware node (ACPI/DT)
> > primary
> > Â	Â* will hold the pset.
> > Â	Â*/
> > -	if (!is_pset_node(fwnode))
> > +	if (is_pset_node(fwnode))
> > +		dev->fwnode = NULL;
> > +	else
> > Â		fwnode = fwnode->secondary;
> > Â	if (!IS_ERR(fwnode) && is_pset_node(fwnode))
> > Â		pset_free_set(to_pset_node(fwnode));
> 
> 
> What if we do the following
> 
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -818,9 +818,13 @@ void device_remove_property_set(struct device
> *dev)
> ÂÂÂÂÂÂÂÂÂ*/
> ÂÂÂÂÂÂÂÂif (!is_pset_node(fwnode))
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂfwnode = fwnode->secondary;
> +
> +ÂÂÂÂÂÂÂ/* Set device fwnode to NULL before we free it */
> +ÂÂÂÂÂÂÂset_secondary_fwnode(dev, NULL);
> +
> +ÂÂÂÂÂÂÂ/* Free property set for the given device */
> ÂÂÂÂÂÂÂÂif (!IS_ERR(fwnode) && is_pset_node(fwnode))
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂpset_free_set(to_pset_node(fwnode));
> -ÂÂÂÂÂÂÂset_secondary_fwnode(dev, NULL);
> Â}
> ÂEXPORT_SYMBOL_GPL(device_remove_property_set);
> Â
> ?
> 

Just noticed that there is another potential bug is hidden, if we call
this function on non-pset fwnode we will silently get fwnode set to
NULL.

Considering this, perhaps better solution is to convert last lines to

if (!IS_ERR(fwnode) && is_pset_node(fwnode)) {
 set_secondary_fwnode(dev, NULL);
 pset_free_set(to_pset_node(fwnode));
}

I didn't check if we do serialize access to fwnode. It might be more
bugs with access to it in racing manner.

-- 
Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.