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

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:	Wed, 24 Feb 2016 00:37:01 +0100
From:	"Rafael J. Wysocki" <rjw@...ysocki.net>
To:	"Shevchenko, Andriy" <andriy.shevchenko@...el.com>
Cc:	"heikki.krogerus@...ux.intel.com" <heikki.krogerus@...ux.intel.com>,
	"mika.westerberg@...ux.intel.com" <mika.westerberg@...ux.intel.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"John.Youn@...opsys.com" <John.Youn@...opsys.com>
Subject: Re: [PATCH 2/2] device property: fix for a case of use-after-free

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@...opsys.com>
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > > ---
> > >  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