Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1339592 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] device property: fix for a case of use-after-free Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-22 16:00 +0100
    Re: [PATCH 2/2] device property: fix for a case of use-after-free Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2016-02-22 16:50 +0100
      Re: [PATCH 2/2] device property: fix for a case of use-after-free "Shevchenko, Andriy" <andriy.shevchenko@intel.com> - 2016-02-22 18:10 +0100
        Re: [PATCH 2/2] device property: fix for a case of use-after-free "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-24 00:40 +0100
        Re: [PATCH 2/2] device property: fix for a case of use-after-free Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-26 09:10 +0100
          Re: [PATCH 2/2] device property: fix for a case of use-after-free Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-26 11:40 +0100

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

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@synopsys.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.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));
-- 
2.7.0

[toc] | [next] | [standalone]

#1339661

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@synopsys.com>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.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);
 
?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1339737

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@synopsys.com>
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.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.

-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
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.

[toc] | [prev] | [next] | [standalone]

#1341180

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@synopsys.com>
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.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

[toc] | [prev] | [next] | [standalone]

#1343892

On Mon, Feb 22, 2016 at 05:04:04PM +0000, Shevchenko, Andriy wrote:
> 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));
> }

After some testing, this seems to work. I'll prepare v2 and use it.

Thanks Andy,

-- 
heikki

[toc] | [prev] | [next] | [standalone]

#1344038

On Fri, Feb 26, 2016 at 10:04:34AM +0200, Heikki Krogerus wrote:
> On Mon, Feb 22, 2016 at 05:04:04PM +0000, Shevchenko, Andriy wrote:
> > 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));
> > }
> 
> After some testing, this seems to work. I'll prepare v2 and use it.

Sorry, I was a bit too hasty with my answer. So that also does not
work in case the pset provides the fwnode.


Thanks,

-- 
heikki

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web