[RESEND PATCH 0/2] device property: fix for two bugs

[RESEND PATCH 0/2] device property: fix for two bugs

[Heikki Krogerus]

Hi Rafael,

I'm resending these for your convenience.

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

There were no better ideas that would have worked from anybody, so
I'm proposing we go forward with these for now.


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 | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

-- 
2.7.0

[RESEND 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

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

[Rafael J. Wysocki]

On Tuesday, March 08, 2016 03:44:36 PM Heikki Krogerus wrote:
> 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>

Applied, thanks!

Rafael

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

[Heikki Krogerus]

In device_remove_property_set(), the secondary fwnode needs
to be cleared before the pset is freed. This fixes a
use-after-free when a property set is providing the primary
fwnode.

Reported-by: John Youn <John.Youn@synopsys.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/property.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index a163f2c..a9df21a9 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -820,11 +820,13 @@ 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));
-	set_secondary_fwnode(dev, NULL);
+		set_secondary_fwnode(dev, NULL);
+	pset_free_set(to_pset_node(fwnode));
 }
 EXPORT_SYMBOL_GPL(device_remove_property_set);
 
-- 
2.7.0

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

[Rafael J. Wysocki]

On Tuesday, March 08, 2016 03:44:37 PM Heikki Krogerus wrote:
> In device_remove_property_set(), the secondary fwnode needs
> to be cleared before the pset is freed. This fixes a
> use-after-free when a property set is providing the primary
> fwnode.
> 
> Reported-by: John Youn <John.Youn@synopsys.com>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/base/property.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index a163f2c..a9df21a9 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -820,11 +820,13 @@ 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;

I don't really like the way you clear dev->fwnode directly here.  set_primary_fwnode(dev, NULL) would be more appropriate IMO.

Also set_secondary_fwnode(dev, NULL) need not be done in that case, because it
doesn't change anything.

Moreover, if the primary node is not pset, the secondary one should only be
cleared if it is pset.

So that would mean

	if (is_pset_node(fwnode)) {
		set_primary_fwnode(dev, NULL);
		pset_free_set(to_pset_node(fwnode));
	} else {
		fwnode = fwnode->secondary;
		if (!IS_ERR(fwnode) && is_pset_node(fwnode)) {
			set_secondary_fwnode(dev, NULL);
			pset_free_set(to_pset_node(fwnode));
		}
	}

Or am I mistaken?


> +	else
>  		fwnode = fwnode->secondary;
>  	if (!IS_ERR(fwnode) && is_pset_node(fwnode))
> -		pset_free_set(to_pset_node(fwnode));
> -	set_secondary_fwnode(dev, NULL);
> +		set_secondary_fwnode(dev, NULL);
> +	pset_free_set(to_pset_node(fwnode));
>  }
>  EXPORT_SYMBOL_GPL(device_remove_property_set);
>  

Thanks,
Rafael

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

[Heikki Krogerus]

Hi Rafael,

> > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > index a163f2c..a9df21a9 100644
> > --- a/drivers/base/property.c
> > +++ b/drivers/base/property.c
> > @@ -820,11 +820,13 @@ 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;
> 
> I don't really like the way you clear dev->fwnode directly here.
> set_primary_fwnode(dev, NULL) would be more appropriate IMO.
> 
> Also set_secondary_fwnode(dev, NULL) need not be done in that case, because it
> doesn't change anything.
> 
> Moreover, if the primary node is not pset, the secondary one should only be
> cleared if it is pset.
> 
> So that would mean
> 
> 	if (is_pset_node(fwnode)) {
> 		set_primary_fwnode(dev, NULL);
> 		pset_free_set(to_pset_node(fwnode));
> 	} else {
> 		fwnode = fwnode->secondary;
> 		if (!IS_ERR(fwnode) && is_pset_node(fwnode)) {
> 			set_secondary_fwnode(dev, NULL);
> 			pset_free_set(to_pset_node(fwnode));
> 		}
> 	}

I have been testing that today, but I'll continue tomorrow. No
problems so far.


Thanks,

-- 
heikki

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

[Heikki Krogerus]

On Wed, Mar 09, 2016 at 04:41:12PM +0200, Heikki Krogerus wrote:
> Hi Rafael,
> 
> > > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > > index a163f2c..a9df21a9 100644
> > > --- a/drivers/base/property.c
> > > +++ b/drivers/base/property.c
> > > @@ -820,11 +820,13 @@ 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;
> > 
> > I don't really like the way you clear dev->fwnode directly here.
> > set_primary_fwnode(dev, NULL) would be more appropriate IMO.
> > 
> > Also set_secondary_fwnode(dev, NULL) need not be done in that case, because it
> > doesn't change anything.
> > 
> > Moreover, if the primary node is not pset, the secondary one should only be
> > cleared if it is pset.
> > 
> > So that would mean
> > 
> > 	if (is_pset_node(fwnode)) {
> > 		set_primary_fwnode(dev, NULL);

So this does create an other problem. We may now end up having ERR_PTR
in the primary fwnode which of course is not considered in the
property handling code, and would need to be fixed similarly as I
fixed the secondary fwnode checking in 1/2 of this series.

But instead of doing that, the whole logic of handing the
primary/secondary fwnodes is probable a little bit broken and should
really be replaced with something better. We can fix these issues one
by one with the code we have now, but it really looks like one fix
will always result into two new problems. Andy promised to look at it.

In the meantime, would it make sense to go ahead with my proposal to
fix the original problem? As a temporary hack?


Thanks,

-- 
heikki

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

[Heikki Krogerus]

On Thu, Mar 10, 2016 at 10:44:45AM +0200, Heikki Krogerus wrote:
> On Wed, Mar 09, 2016 at 04:41:12PM +0200, Heikki Krogerus wrote:
> > Hi Rafael,
> > 
> > > > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > > > index a163f2c..a9df21a9 100644
> > > > --- a/drivers/base/property.c
> > > > +++ b/drivers/base/property.c
> > > > @@ -820,11 +820,13 @@ 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;
> > > 
> > > I don't really like the way you clear dev->fwnode directly here.
> > > set_primary_fwnode(dev, NULL) would be more appropriate IMO.
> > > 
> > > Also set_secondary_fwnode(dev, NULL) need not be done in that case, because it
> > > doesn't change anything.
> > > 
> > > Moreover, if the primary node is not pset, the secondary one should only be
> > > cleared if it is pset.
> > > 
> > > So that would mean
> > > 
> > > 	if (is_pset_node(fwnode)) {
> > > 		set_primary_fwnode(dev, NULL);
> 
> So this does create an other problem. We may now end up having ERR_PTR
> in the primary fwnode which of course is not considered in the
> property handling code, and would need to be fixed similarly as I
> fixed the secondary fwnode checking in 1/2 of this series.
> 
> But instead of doing that, the whole logic of handing the
> primary/secondary fwnodes is probable a little bit broken and should
> really be replaced with something better. We can fix these issues one
> by one with the code we have now, but it really looks like one fix
> will always result into two new problems. Andy promised to look at it.
> 
> In the meantime, would it make sense to go ahead with my proposal to
> fix the original problem? As a temporary hack?

Scratch that last question. I'll just prepare a patch with your
proposal and in the same patch change the checking of the primary
fwnode into !IS_ERR_OR_NULL(fwnode). I think it should work for now.


Thanks,

-- 
heikki