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@xxxxxxxxxxxx>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
