[PATCH 0/2] device property: fix for couple of bugs

[PATCH 0/2] device property: fix for couple of bugs

[Heikki Krogerus]

Hi,

The fwnode->secondary is causing problems when a property_set is
providing the primary fwnode.

Both of these fixes are a bit clumsy looking IMO, but I didn't have
better ideas. I think the second one fixing the use-after-free bug
should ideally be taken care of in set_secondary_fwnode() instead of
device_remove_property_set(), but I didn't have any ideas how to do
that.


Heikki Krogerus (2):
  device property: fwnode->secondary may contain ERR_PTR(-ENODEV)
  device property: fix for a case of use-after-free

 drivers/base/property.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

-- 
2.7.0

[PATCH 1/2] device property: fwnode->secondary may contain ERR_PTR(-ENODEV)

[Heikki Krogerus]

This fixes BUG that is trickered when fwnode->secondary is not null,
but has ERR_PTR(-ENODEV) instead.

BUG: unable to handle kernel paging request at ffffffffffffffed
IP: [<ffffffff81677b86>] __fwnode_property_read_string+0x26/0x160
PGD 200e067 PUD 2010067 PMD 0
Oops: 0000 [#1] SMP KASAN
Modules linked in: dwc3_pci(+) dwc3
CPU: 0 PID: 1138 Comm: modprobe Not tainted 4.5.0-rc5+ #61
task: ffff88015aaf5b00 ti: ffff88007b958000 task.ti: ffff88007b958000
RIP: 0010:[<ffffffff81677b86>]  [<ffffffff81677b86>] __fwnode_property_read_string+0x26/0x160
RSP: 0018:ffff88007b95eff8  EFLAGS: 00010246
RAX: fffffbfffffffffd RBX: ffffffffffffffed RCX: ffff88015999cd37
RDX: dffffc0000000000 RSI: ffffffff81e11bc0 RDI: ffffffffffffffed
RBP: ffff88007b95f020 R08: 0000000000000000 R09: 0000000000000000
R10: ffff88007b90f7cf R11: 0000000000000000 R12: ffff88007b95f0a0
R13: 00000000fffffffa R14: ffffffff81e11bc0 R15: ffff880159ea37a0
FS:  00007ff35f46c700(0000) GS:ffff88015b800000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: ffffffffffffffed CR3: 000000007b8be000 CR4: 00000000001006f0
Stack:
 ffff88015999cd20 ffffffff81e11bc0 ffff88007b95f0a0 ffff88007b383dd8
 ffff880159ea37a0 ffff88007b95f048 ffffffff81677d03 ffff88007b952460
 ffffffff81e11bc0 ffff88007b95f0a0 ffff88007b95f070 ffffffff81677d40
Call Trace:
 [<ffffffff81677d03>] fwnode_property_read_string+0x43/0x50
 [<ffffffff81677d40>] device_property_read_string+0x30/0x40
...

Fixes: 362c0b30249e (device property: Fallback to secondary fwnode if primary misses the property)
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/property.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index c359351..a163f2c 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -218,7 +218,7 @@ bool fwnode_property_present(struct fwnode_handle *fwnode, const char *propname)
 	bool ret;
 
 	ret = __fwnode_property_present(fwnode, propname);
-	if (ret == false && fwnode && fwnode->secondary)
+	if (ret == false && fwnode && !IS_ERR_OR_NULL(fwnode->secondary))
 		ret = __fwnode_property_present(fwnode->secondary, propname);
 	return ret;
 }
@@ -423,7 +423,7 @@ EXPORT_SYMBOL_GPL(device_property_match_string);
 	int _ret_;									\
 	_ret_ = FWNODE_PROP_READ(_fwnode_, _propname_, _type_, _proptype_,		\
 				 _val_, _nval_);					\
-	if (_ret_ == -EINVAL && _fwnode_ && _fwnode_->secondary)			\
+	if (_ret_ == -EINVAL && _fwnode_ && !IS_ERR_OR_NULL(_fwnode_->secondary))	\
 		_ret_ = FWNODE_PROP_READ(_fwnode_->secondary, _propname_, _type_,	\
 				_proptype_, _val_, _nval_);				\
 	_ret_;										\
@@ -593,7 +593,7 @@ int fwnode_property_read_string_array(struct fwnode_handle *fwnode,
 	int ret;
 
 	ret = __fwnode_property_read_string_array(fwnode, propname, val, nval);
-	if (ret == -EINVAL && fwnode && fwnode->secondary)
+	if (ret == -EINVAL && fwnode && !IS_ERR_OR_NULL(fwnode->secondary))
 		ret = __fwnode_property_read_string_array(fwnode->secondary,
 							  propname, val, nval);
 	return ret;
@@ -621,7 +621,7 @@ int fwnode_property_read_string(struct fwnode_handle *fwnode,
 	int ret;
 
 	ret = __fwnode_property_read_string(fwnode, propname, val);
-	if (ret == -EINVAL && fwnode && fwnode->secondary)
+	if (ret == -EINVAL && fwnode && !IS_ERR_OR_NULL(fwnode->secondary))
 		ret = __fwnode_property_read_string(fwnode->secondary,
 						    propname, val);
 	return ret;
-- 
2.7.0

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

[Heikki Krogerus]

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

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

[Andy Shevchenko]

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

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@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.

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@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

Re: [PATCH 0/2] device property: fix for couple of bugs

[Rafael J. Wysocki]

On Monday, February 22, 2016 04:50:39 PM Heikki Krogerus wrote:
> Hi,
> 
> The fwnode->secondary is causing problems when a property_set is
> providing the primary fwnode.
> 
> Both of these fixes are a bit clumsy looking IMO, but I didn't have
> better ideas. I think the second one fixing the use-after-free bug
> should ideally be taken care of in set_secondary_fwnode() instead of
> device_remove_property_set(), but I didn't have any ideas how to do
> that.

Can you please CC device property framework patches to linux-acpi?

They are somewhat easier to handle this way.

Thanks,
Rafael

Re: [PATCH 0/2] device property: fix for couple of bugs

[Heikki Krogerus]

On Wed, Feb 24, 2016 at 12:37:43AM +0100, Rafael J. Wysocki wrote:
> On Monday, February 22, 2016 04:50:39 PM Heikki Krogerus wrote:
> > Hi,
> > 
> > The fwnode->secondary is causing problems when a property_set is
> > providing the primary fwnode.
> > 
> > Both of these fixes are a bit clumsy looking IMO, but I didn't have
> > better ideas. I think the second one fixing the use-after-free bug
> > should ideally be taken care of in set_secondary_fwnode() instead of
> > device_remove_property_set(), but I didn't have any ideas how to do
> > that.
> 
> Can you please CC device property framework patches to linux-acpi?
> 
> They are somewhat easier to handle this way.

Will do.

Thanks,

-- 
heikki

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

[Heikki Krogerus]

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

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

[Heikki Krogerus]

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