Skip to contentSkip to conversation

staging: greybus: spilib: fix use-after-free after deregistration

PATCH In review Johan Hovold 3 participants 5 messages Oct 29, 2017

Subsystems (6)
Stable branch Staging SPI Greybus +2 more subsystems
lore
Share & tools
lore

Johan Hovold

author maintainer
#0 lore raw

PATCH 1/1 staging: greybus: spilib: fix use-after-free after deregistration

Remove erroneous spi_master_put() after controller deregistration which would access the already freed spi controller.

Note that spi_unregister_master() drops our only controller reference.

Fixes ba3e67001b42 ("greybus: SPI: convert to a gpbridge driver")
Cc stable <[email protected]> # 4.9
Cc Greg Kroah-Hartman <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/staging/greybus/spilib.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/staging/greybus/spilib.c +5 -3
index e97b19148497..1e7321a1404c 100644
@@ -544,11 +544,14 @@ int gb_spilib_master_init(struct gb_connection *connection, struct device *dev,
 
 	return 0;
 
-exit_spi_unregister:
-	spi_unregister_master(master);
 exit_spi_put:
 	spi_master_put(master);
 
+	return ret;
+
+exit_spi_unregister:
+	spi_unregister_master(master);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(gb_spilib_master_init);

@@ -558,7 +561,6 @@ void gb_spilib_master_exit(struct gb_connection *connection)
 	struct spi_master *master = gb_connection_get_data(connection);
 
 	spi_unregister_master(master);
-	spi_master_put(master);
 }
 EXPORT_SYMBOL_GPL(gb_spilib_master_exit);
Signature · 1 line
2.14.3
Acked-by on v1 1/1
#1 lore raw
Quoted 35 lines · Johan Hovold

Remove erroneous spi_master_put() after controller deregistration which would access the already freed spi controller.

Note that spi_unregister_master() drops our only controller reference.

Fixes ba3e67001b42 ("greybus: SPI: convert to a gpbridge driver")
Cc stable <[email protected]> # 4.9
Cc Greg Kroah-Hartman <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/staging/greybus/spilib.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/staging/greybus/spilib.c +5 -2
index e97b19148497..1e7321a1404c 100644
@@ -544,11 +544,14 @@ int gb_spilib_master_init(struct gb_connection *connection, struct device *dev,
 
 	return 0;
 
-exit_spi_unregister:
-	spi_unregister_master(master);
 exit_spi_put:
 	spi_master_put(master);
 
+	return ret;
+
+exit_spi_unregister:
+	spi_unregister_master(master);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(gb_spilib_master_init);

drivers/staging/greybus/spilib.c void gb_spilib_master_exit(struct gb_connection *connection) · line 565

Quoted 6 lines
drivers/staging/greybus/spilib.c +0 -1
@@ -558,7 +561,6 @@ void gb_spilib_master_exit(struct gb_connection *connection)
 	struct spi_master *master = gb_connection_get_data(connection);
 
 	spi_unregister_master(master);
-	spi_master_put(master);
 }
 EXPORT_SYMBOL_GPL(gb_spilib_master_exit);
Acked-by Viresh Kumar <[email protected]>

While looking at this I think I found another problem (I will send it as a separate patch later on) and this fixes it:

drivers/spi/spi.c +1 -1
index 6e65524cbfd9..af7ca751b4f7 100644
@@ -2261,11 +2261,11 @@ void spi_unregister_controller(struct spi_controller *ctlr)
        mutex_unlock(&board_lock);
 
        dummy = device_for_each_child(&ctlr->dev, NULL, __unregister);
-       device_unregister(&ctlr->dev);
        /* free bus id */
        mutex_lock(&board_lock);
        idr_remove(&spi_master_idr, ctlr->bus_num);
        mutex_unlock(&board_lock);
+       device_unregister(&ctlr->dev);
 }
 EXPORT_SYMBOL_GPL(spi_unregister_controller);
Signature · 1 line
viresh
author maintainer
#2 lore raw
Quoted 11 lines · Viresh Kumar
Quoted 9 lines · Johan Hovold

Remove erroneous spi_master_put() after controller deregistration which would access the already freed spi controller.

Note that spi_unregister_master() drops our only controller reference.

Fixes ba3e67001b42 ("greybus: SPI: convert to a gpbridge driver")
Cc stable <[email protected]> # 4.9
Cc Greg Kroah-Hartman <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
Quoted 4 lines
Acked-by Viresh Kumar <[email protected]>

While looking at this I think I found another problem (I will send it as a separate patch later on) and this fixes it:

That's right, and I already posted a fix for that use-after-free:

	https://lkml.kernel.org/r/[email protected]

Thanks,
Johan

maintainer Reviewed-by on v1 1/1
#4 lore raw
In reply to Johan Hovold

Hi Johan,
Thanks for the patch.

Quoted 9 lines · Johan Hovold

Remove erroneous spi_master_put() after controller deregistration which would access the already freed spi controller.

Note that spi_unregister_master() drops our only controller reference.

Fixes ba3e67001b42 ("greybus: SPI: convert to a gpbridge driver")
Cc stable <[email protected]> # 4.9
Cc Greg Kroah-Hartman <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
Reviewed-by Rui Miguel Silva <[email protected]>

---
Cheers,
Rui

Quoted 32 lines
---
 drivers/staging/greybus/spilib.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/staging/greybus/spilib.c +5 -3
index e97b19148497..1e7321a1404c 100644
@@ -544,11 +544,14 @@ int gb_spilib_master_init(struct gb_connection *connection, struct device *dev,
 
 	return 0;
 
-exit_spi_unregister:
-	spi_unregister_master(master);
 exit_spi_put:
 	spi_master_put(master);
 
+	return ret;
+
+exit_spi_unregister:
+	spi_unregister_master(master);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(gb_spilib_master_init);

@@ -558,7 +561,6 @@ void gb_spilib_master_exit(struct gb_connection *connection)
 	struct spi_master *master = gb_connection_get_data(connection);
 
 	spi_unregister_master(master);
-	spi_master_put(master);
 }
 EXPORT_SYMBOL_GPL(gb_spilib_master_exit);