From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752849AbcBVREK (ORCPT ); Mon, 22 Feb 2016 12:04:10 -0500 Received: from mga01.intel.com ([192.55.52.88]:59101 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752198AbcBVREJ (ORCPT ); Mon, 22 Feb 2016 12:04:09 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,485,1449561600"; d="scan'208";a="918581386" From: "Shevchenko, Andriy" To: "heikki.krogerus@linux.intel.com" , "rjw@rjwysocki.net" CC: "mika.westerberg@linux.intel.com" , "linux-kernel@vger.kernel.org" , "John.Youn@synopsys.com" Subject: Re: [PATCH 2/2] device property: fix for a case of use-after-free Thread-Topic: [PATCH 2/2] device property: fix for a case of use-after-free Thread-Index: AQHRbZMIiU/HacFgv0mh3MOzOrENXQ== Date: Mon, 22 Feb 2016 17:04:04 +0000 Message-ID: <1456160688.13244.30.camel@intel.com> References: <1456152641-127948-1-git-send-email-heikki.krogerus@linux.intel.com> <1456152641-127948-3-git-send-email-heikki.krogerus@linux.intel.com> <1456155650.13244.24.camel@linux.intel.com> In-Reply-To: <1456155650.13244.24.camel@linux.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.237.72.86] Content-Type: text/plain; charset="utf-8" Content-ID: <1DA46C49F51EBC4B8261382441CE5434@intel.com> MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id u1MH4GEk003626 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 > > Signed-off-by: Heikki Krogerus > > --- > >  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 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.