device property: fix for a case of use-after-free

Hi Heikki Krogerus,

The patch 0d67e0fa1664: "device property: fix for a case of use-after-free" from Mar 10, 2016, has an issue.

Assume "fwnode" is an ERR_PTR>

drivers/base/property.c
204 static bool __fwnode_property_present(struct fwnode_handle *fwnode,
205 const char *propname)
206 {
207 if (is_of_node(fwnode))

                               ^^^^^^

We dereference it here.

   208                  return of_property_read_bool(to_of_node(fwnode), propname);
   209          else if (is_acpi_node(fwnode))
   210                  return !acpi_node_prop_get(fwnode, propname, NULL);
   211          else if (is_pset_node(fwnode))
   212                  return !!pset_prop_get(to_pset_node(fwnode), propname);
   213          return false;

Some of these depend on the .config but I don't see a path through this function where fwnode can be an ERR_PTR and we don't oops.

214 }
215
216 /**

   217   * fwnode_property_present - check if a property of a firmware node is present
   218   * @fwnode: Firmware node whose property to check
   219   * @propname: Name of the property
   220   */
   221  bool fwnode_property_present(struct fwnode_handle *fwnode, const char *propname)
   222  {
   223          bool ret;
   224  
   225          ret = __fwnode_property_present(fwnode, propname);
                                                ^^^^^^^

We oops here.

226 if (ret = false && !IS_ERR_OR_NULL(fwnode) &&

                                     ^^^^^^^^^^^^^^^^^^^^^^

This check for IS_ERR is too late because we already oopsed on the line before.

   227              !IS_ERR_OR_NULL(fwnode->secondary))
   228                  ret = __fwnode_property_present(fwnode->secondary, propname);
   229          return ret;
   230  }

regards,
dan carpenter

Hi Dan,

Hmm. It looks like that function also just checks fwnode and not IS_ERR_OR_NULL(fwnode)..

Does this fix the issue for you? I noticed that the other types don't check it any more then of:

index 9b1a65d..7f692ac 100644
@@ -21,7 +21,7 @@
 
 static inline bool is_pset_node(struct fwnode_handle *fwnode)
 {
-       return fwnode && fwnode->type = FWNODE_PDATA;
+       return !IS_ERR_OR_NULL(fwnode) && fwnode->type = FWNODE_PDATA;
 }
 
 static inline struct property_set *to_pset_node(struct fwnode_handle *fwnode)

index 14362a8..3a93250 100644
@@ -394,13 +394,13 @@ struct acpi_data_node {
 
 static inline bool is_acpi_node(struct fwnode_handle *fwnode)
 {
-       return fwnode && (fwnode->type = FWNODE_ACPI
+       return !IS_ERR_OR_NULL(fwnode) && (fwnode->type = FWNODE_ACPI
                || fwnode->type = FWNODE_ACPI_DATA);
 }
 
 static inline bool is_acpi_device_node(struct fwnode_handle *fwnode)
 {
-       return fwnode && fwnode->type = FWNODE_ACPI;
+       return !IS_ERR_OR_NULL(fwnode) && fwnode->type = FWNODE_ACPI;
 }
 
 static inline struct acpi_device *to_acpi_device_node(struct fwnode_handle *fwnode)

index 7fcb681..3175803 100644
@@ -133,7 +133,7 @@ void of_core_init(void);
 
 static inline bool is_of_node(struct fwnode_handle *fwnode)
 {
-       return fwnode && fwnode->type = FWNODE_OF;
+       return !IS_ERR_OR_NULL(fwnode) && fwnode->type = FWNODE_OF;
 }
 
 static inline struct device_node *to_of_node(struct fwnode_handle *fwnode)

Thanks,

heikki

Sounds ok to me.

regards,
dan carpenter