Re: [PATCH] staging: greybus: spilib: fix use-after-free after deregistration
Hi Johan, Thanks for the patch. On Sun 29 Oct 2017 at 12:01, Johan Hovoldwrote: > 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 # 4.9 > Cc: Greg Kroah-Hartman > Signed-off-by: Johan Hovold Reviewed-by: Rui Miguel Silva --- Cheers, Rui > --- > drivers/staging/greybus/spilib.c | 8 +--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/staging/greybus/spilib.c > b/drivers/staging/greybus/spilib.c > index e97b19148497..1e7321a1404c 100644 > --- a/drivers/staging/greybus/spilib.c > +++ b/drivers/staging/greybus/spilib.c > @@ -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);
Re: [PATCH] staging: greybus: spilib: fix use-after-free after deregistration
Hi Johan, Thanks for the patch. On Sun 29 Oct 2017 at 12:01, Johan Hovold wrote: > 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 # 4.9 > Cc: Greg Kroah-Hartman > Signed-off-by: Johan Hovold Reviewed-by: Rui Miguel Silva --- Cheers, Rui > --- > drivers/staging/greybus/spilib.c | 8 +--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/staging/greybus/spilib.c > b/drivers/staging/greybus/spilib.c > index e97b19148497..1e7321a1404c 100644 > --- a/drivers/staging/greybus/spilib.c > +++ b/drivers/staging/greybus/spilib.c > @@ -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);
Re: [PATCH] staging: greybus: spilib: fix use-after-free after deregistration
On 29-10-17, 13:51, Johan Hovold wrote: > That's right, and I already posted a fix for that use-after-free: > > https://lkml.kernel.org/r/20171029115625.32385-1-jo...@kernel.org Great :) -- viresh
Re: [PATCH] staging: greybus: spilib: fix use-after-free after deregistration
On 29-10-17, 13:51, Johan Hovold wrote: > That's right, and I already posted a fix for that use-after-free: > > https://lkml.kernel.org/r/20171029115625.32385-1-jo...@kernel.org Great :) -- viresh
Re: [PATCH] staging: greybus: spilib: fix use-after-free after deregistration
On Sun, Oct 29, 2017 at 06:13:28PM +0530, Viresh Kumar wrote: > On 29-10-17, 13:01, Johan Hovold wrote: > > 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# 4.9 > > Cc: Greg Kroah-Hartman > > Signed-off-by: Johan Hovold > Acked-by: Viresh Kumar > > 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/20171029115625.32385-1-jo...@kernel.org Thanks, Johan
Re: [PATCH] staging: greybus: spilib: fix use-after-free after deregistration
On Sun, Oct 29, 2017 at 06:13:28PM +0530, Viresh Kumar wrote: > On 29-10-17, 13:01, Johan Hovold wrote: > > 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 # 4.9 > > Cc: Greg Kroah-Hartman > > Signed-off-by: Johan Hovold > Acked-by: Viresh Kumar > > 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/20171029115625.32385-1-jo...@kernel.org Thanks, Johan
Re: [PATCH] staging: greybus: spilib: fix use-after-free after deregistration
On 29-10-17, 13:01, Johan Hovold wrote: > 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# 4.9 > Cc: Greg Kroah-Hartman > Signed-off-by: Johan Hovold > --- > drivers/staging/greybus/spilib.c | 8 +--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/staging/greybus/spilib.c > b/drivers/staging/greybus/spilib.c > index e97b19148497..1e7321a1404c 100644 > --- a/drivers/staging/greybus/spilib.c > +++ b/drivers/staging/greybus/spilib.c > @@ -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); Acked-by: Viresh Kumar While looking at this I think I found another problem (I will send it as a separate patch later on) and this fixes it: diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 6e65524cbfd9..af7ca751b4f7 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -2261,11 +2261,11 @@ void spi_unregister_controller(struct spi_controller *ctlr) mutex_unlock(_lock); dummy = device_for_each_child(>dev, NULL, __unregister); - device_unregister(>dev); /* free bus id */ mutex_lock(_lock); idr_remove(_master_idr, ctlr->bus_num); mutex_unlock(_lock); + device_unregister(>dev); } EXPORT_SYMBOL_GPL(spi_unregister_controller); -- viresh
Re: [PATCH] staging: greybus: spilib: fix use-after-free after deregistration
On 29-10-17, 13:01, Johan Hovold wrote: > 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 # 4.9 > Cc: Greg Kroah-Hartman > Signed-off-by: Johan Hovold > --- > drivers/staging/greybus/spilib.c | 8 +--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/staging/greybus/spilib.c > b/drivers/staging/greybus/spilib.c > index e97b19148497..1e7321a1404c 100644 > --- a/drivers/staging/greybus/spilib.c > +++ b/drivers/staging/greybus/spilib.c > @@ -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); Acked-by: Viresh Kumar While looking at this I think I found another problem (I will send it as a separate patch later on) and this fixes it: diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 6e65524cbfd9..af7ca751b4f7 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -2261,11 +2261,11 @@ void spi_unregister_controller(struct spi_controller *ctlr) mutex_unlock(_lock); dummy = device_for_each_child(>dev, NULL, __unregister); - device_unregister(>dev); /* free bus id */ mutex_lock(_lock); idr_remove(_master_idr, ctlr->bus_num); mutex_unlock(_lock); + device_unregister(>dev); } EXPORT_SYMBOL_GPL(spi_unregister_controller); -- viresh