Messages in this thread | | | From | "Shevchenko, Andriy" <> | Subject | Re: [PATCH 2/2] device property: fix for a case of use-after-free | Date | Mon, 22 Feb 2016 17:04:04 +0000 |
| |
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.
| |