greybus: interface: fix interface-registration race

Fix race with user space when registering the interface.

The interface was registered before having been fully initialised,
something which could lead to user space accessing not-yet-initialised
attribute values (e.g. zero vendor and product ids or empty vendor and
product strings).

Note that this is also needed to be able to let attribute visibility
depend on manifest data (e.g. interface unlock).

Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>

diff --git a/drivers/staging/greybus/interface.c b/drivers/staging/greybus/interface.c
index 98a1ba1e485ddddc3421952a4df125258dd2c5dc..1c6a8a4ac4b5d68020320d69d014d96275cc32c2 100644 (file)
--- a/drivers/staging/greybus/interface.c
+++ b/drivers/staging/greybus/interface.c
@@ -85,7 +85,6 @@ struct gb_interface *gb_interface_create(struct gb_host_device *hd,
                                         u8 interface_id)
 {
        struct gb_interface *intf;
-       int retval;
 
        intf = kzalloc(sizeof(*intf), GFP_KERNEL);
        if (!intf)
@@ -107,21 +106,11 @@ struct gb_interface *gb_interface_create(struct gb_host_device *hd,
        device_initialize(&intf->dev);
        dev_set_name(&intf->dev, "%d-%d", hd->bus_id, interface_id);
 
-       retval = device_add(&intf->dev);
-       if (retval) {
-               pr_err("failed to add interface %u\n", interface_id);
-               goto free_intf;
-       }
-
        spin_lock_irq(&gb_interfaces_lock);
        list_add(&intf->links, &hd->interfaces);
        spin_unlock_irq(&gb_interfaces_lock);
 
        return intf;
-
-free_intf:
-       put_device(&intf->dev);
-       return NULL;
 }
 
 /*
@@ -132,17 +121,20 @@ void gb_interface_remove(struct gb_interface *intf)
        struct gb_bundle *bundle;
        struct gb_bundle *next;
 
-       spin_lock_irq(&gb_interfaces_lock);
-       list_del(&intf->links);
-       spin_unlock_irq(&gb_interfaces_lock);
-
        list_for_each_entry_safe(bundle, next, &intf->bundles, links)
                gb_bundle_destroy(bundle);
 
+       if (device_is_registered(&intf->dev))
+               device_del(&intf->dev);
+
        if (intf->control)
                gb_connection_destroy(intf->control->connection);
 
-       device_unregister(&intf->dev);
+       spin_lock_irq(&gb_interfaces_lock);
+       list_del(&intf->links);
+       spin_unlock_irq(&gb_interfaces_lock);
+
+       put_device(&intf->dev);
 }
 
 void gb_interfaces_remove(struct gb_host_device *hd)
@@ -216,7 +208,13 @@ int gb_interface_init(struct gb_interface *intf, u8 device_id)
                goto free_manifest;
        }
 
-       /* Register the interface bundles. */
+       /* Register the interface and its bundles. */
+       ret = device_add(&intf->dev);
+       if (ret) {
+               dev_err(&intf->dev, "failed to register interface: %d\n", ret);
+               goto free_manifest;
+       }
+
        list_for_each_entry_safe_reverse(bundle, tmp, &intf->bundles, links) {
                ret = gb_bundle_add(bundle);
                if (ret) {