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

[Rafael J. Wysocki]

On Monday, February 22, 2016 05:04:04 PM Shevchenko, Andriy wrote:
> 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.

The core doesn't serialize those.

If the callers need serialization, they should use some locking around
those calls.

Thanks,
Rafael