drivers/spi/mpc52xx_psc_spi.c, potential use after free

drivers/spi/mpc52xx_psc_spi.c, potential use after free

[Sebastian Siewior]

Hello Dragos,

Your mpc52xx_psc_spi_do_remove() from current git:

| static int __exit mpc52xx_psc_spi_do_remove(struct device *dev)
| {
|         struct spi_master *master = dev_get_drvdata(dev);
|         struct mpc52xx_psc_spi *mps = spi_master_get_devdata(master);
| 
|         flush_workqueue(mps->workqueue);
|         destroy_workqueue(mps->workqueue);
|         spi_unregister_master(master);
|         free_irq(mps->irq, mps);
|         if (mps->psc)
|                 iounmap(mps->psc);
| 
|         return 0;
| }

It looks to me that spi_unregister_master() frees your mps struct and
the following free_irq() and iounmap() touches deallocated memory.
Might this be correct?

Sebastian

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[PATCH] Re: drivers/spi/mpc52xx_psc_spi.c, potential use after free

[Dragos Carp]

Fix mpc52xx_psc_spi_do_remove() use of drvdata after spi master
unregister.

Signed-off-by: Dragos Carp <dragos.carp-+ybMorV4k0dBDgjK7y7TUQ@public.gmane.org>
---

 mpc52xx_psc_spi.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/mpc52xx_psc_spi.c
b/drivers/spi/mpc52xx_psc_spi.c
index 7051e6c..a4463ba 100644
--- a/drivers/spi/mpc52xx_psc_spi.c
+++ b/drivers/spi/mpc52xx_psc_spi.c
@@ -534,10 +534,10 @@ static int __exit mpc52xx_psc_spi_do_remove(struct
device *dev)
 
        flush_workqueue(mps->workqueue);
        destroy_workqueue(mps->workqueue);
-       spi_unregister_master(master);
        free_irq(mps->irq, mps);
        if (mps->psc)
                iounmap(mps->psc);
+       spi_unregister_master(master);
 
        return 0;
 }



-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH] Re: drivers/spi/mpc52xx_psc_spi.c, potential use after free

[Sebastian Siewior]

* Dragos Carp | 2008-01-10 15:19:06 [+0100]:

>Fix mpc52xx_psc_spi_do_remove() use of drvdata after spi master
>unregister.
>
>Signed-off-by: Dragos Carp <dragos.carp-+ybMorV4k0dBDgjK7y7TUQ@public.gmane.org>
Acked-by Sebastian Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>

>---
>
> mpc52xx_psc_spi.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/spi/mpc52xx_psc_spi.c
>b/drivers/spi/mpc52xx_psc_spi.c
>index 7051e6c..a4463ba 100644
>--- a/drivers/spi/mpc52xx_psc_spi.c
>+++ b/drivers/spi/mpc52xx_psc_spi.c
>@@ -534,10 +534,10 @@ static int __exit mpc52xx_psc_spi_do_remove(struct
>device *dev)
> 
>        flush_workqueue(mps->workqueue);
>        destroy_workqueue(mps->workqueue);
>-       spi_unregister_master(master);
>        free_irq(mps->irq, mps);
>        if (mps->psc)
>                iounmap(mps->psc);
>+       spi_unregister_master(master);
> 
>        return 0;
> }
This should work since you don't touch HW in cleanup.
Another question: It seems you don't use any release_mem_region() /
request_mem_region(). Are those hidden somewhere behind DT parsing or
you don't do them at all?

Sebastian

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH] Re: drivers/spi/mpc52xx_psc_spi.c, potential use after free

[David Brownell]

On Thursday 10 January 2008, Dragos Carp wrote:

> --- a/drivers/spi/mpc52xx_psc_spi.c
> +++ b/drivers/spi/mpc52xx_psc_spi.c
> @@ -534,10 +534,10 @@ static int __exit mpc52xx_psc_spi_do_remove(struct
> device *dev)
>  
>         flush_workqueue(mps->workqueue);

Between here ...

>         destroy_workqueue(mps->workqueue);
> -       spi_unregister_master(master);
>         free_irq(mps->irq, mps);
>         if (mps->psc)
>                 iounmap(mps->psc);

... and here, drivers can submit I/O requests.  Which will then
be completely lost!!

Can you provide a proper shutdown sequence for this driver?

ISTR that some other adapters grab an extra reference to the
adapter so that they can unregister first (getting rid of
the spi_device children, unbinding their drivers, and ensuring
no more incoming requests) and then clean up resources stored
with the driver, and then finally spi_master_put().

- Dave


> +       spi_unregister_master(master);
>  
>         return 0;
>  }
> 
> 
> 

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace