| Message ID | 20190624140731.24080-1-TheSven73@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | dmaengine: imx-sdma: fix use-after-free on probe error path | expand |
Reviewed-by: Robin Gong <yibin.gong@nxp.com> > -----Original Message----- > From: Sven Van Asbroeck <thesven73@gmail.com> > Subject: [PATCH] dmaengine: imx-sdma: fix use-after-free on probe error path > > If probe() fails anywhere beyond the point where > sdma_get_firmware() is called, then a kernel oops may occur. > > Problematic sequence of events: > 1. probe() calls sdma_get_firmware(), which schedules the > firmware callback to run when firmware becomes available, > using the sdma instance structure as the context 2. probe() encounters an > error, which deallocates the > sdma instance structure > 3. firmware becomes available, firmware callback is > called with deallocated sdma instance structure 4. use after free - kernel > oops ! > > Solution: only attempt to load firmware when we're certain that probe() will > succeed. This guarantees that the firmware callback's context will remain valid. > > Note that the remove() path is unaffected by this issue: the firmware loader will > increment the driver module's use count, ensuring that the module cannot be > unloaded while the firmware callback is pending or running. > > To: Robin Gong <yibin.gong@nxp.com> > Cc: Vinod Koul <vkoul@kernel.org> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Shawn Guo <shawnguo@kernel.org> > Cc: Sascha Hauer <s.hauer@pengutronix.de> > Cc: Pengutronix Kernel Team <kernel@pengutronix.de> > Cc: Fabio Estevam <festevam@gmail.com> > Cc: NXP Linux Team <linux-imx@nxp.com> > Cc: dmaengine@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com> > --- > drivers/dma/imx-sdma.c | 48 ++++++++++++++++++++++++------------------ > 1 file changed, 27 insertions(+), 21 deletions(-) > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index > 99d9f431ae2c..3f0f41d16e1c 100644 > --- a/drivers/dma/imx-sdma.c > +++ b/drivers/dma/imx-sdma.c > @@ -2096,27 +2096,6 @@ static int sdma_probe(struct platform_device > *pdev) > if (pdata && pdata->script_addrs) > sdma_add_scripts(sdma, pdata->script_addrs); > > - if (pdata) { > - ret = sdma_get_firmware(sdma, pdata->fw_name); > - if (ret) > - dev_warn(&pdev->dev, "failed to get firmware from platform > data\n"); > - } else { > - /* > - * Because that device tree does not encode ROM script address, > - * the RAM script in firmware is mandatory for device tree > - * probe, otherwise it fails. > - */ > - ret = of_property_read_string(np, "fsl,sdma-ram-script-name", > - &fw_name); > - if (ret) > - dev_warn(&pdev->dev, "failed to get firmware name\n"); > - else { > - ret = sdma_get_firmware(sdma, fw_name); > - if (ret) > - dev_warn(&pdev->dev, "failed to get firmware from device > tree\n"); > - } > - } > - > sdma->dma_device.dev = &pdev->dev; > > sdma->dma_device.device_alloc_chan_resources = > sdma_alloc_chan_resources; @@ -2161,6 +2140,33 @@ static int > sdma_probe(struct platform_device *pdev) > of_node_put(spba_bus); > } > > + /* > + * Kick off firmware loading as the very last step: > + * attempt to load firmware only if we're not on the error path, because > + * the firmware callback requires a fully functional and allocated sdma > + * instance. > + */ > + if (pdata) { > + ret = sdma_get_firmware(sdma, pdata->fw_name); > + if (ret) > + dev_warn(&pdev->dev, "failed to get firmware from platform > data\n"); > + } else { > + /* > + * Because that device tree does not encode ROM script address, > + * the RAM script in firmware is mandatory for device tree > + * probe, otherwise it fails. > + */ > + ret = of_property_read_string(np, "fsl,sdma-ram-script-name", > + &fw_name); > + if (ret) > + dev_warn(&pdev->dev, "failed to get firmware name\n"); > + else { > + ret = sdma_get_firmware(sdma, fw_name); > + if (ret) > + dev_warn(&pdev->dev, "failed to get firmware from device > tree\n"); > + } > + } > + > return 0; > > err_register: > -- > 2.17.1
On 24-06-19, 10:07, Sven Van Asbroeck wrote: > If probe() fails anywhere beyond the point where > sdma_get_firmware() is called, then a kernel oops may occur. > > Problematic sequence of events: > 1. probe() calls sdma_get_firmware(), which schedules the > firmware callback to run when firmware becomes available, > using the sdma instance structure as the context > 2. probe() encounters an error, which deallocates the > sdma instance structure > 3. firmware becomes available, firmware callback is > called with deallocated sdma instance structure > 4. use after free - kernel oops ! > > Solution: only attempt to load firmware when we're certain > that probe() will succeed. This guarantees that the firmware > callback's context will remain valid. > > Note that the remove() path is unaffected by this issue: the > firmware loader will increment the driver module's use count, > ensuring that the module cannot be unloaded while the > firmware callback is pending or running. > > To: Robin Gong <yibin.gong@nxp.com> > Cc: Vinod Koul <vkoul@kernel.org> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Shawn Guo <shawnguo@kernel.org> > Cc: Sascha Hauer <s.hauer@pengutronix.de> > Cc: Pengutronix Kernel Team <kernel@pengutronix.de> > Cc: Fabio Estevam <festevam@gmail.com> > Cc: NXP Linux Team <linux-imx@nxp.com> > Cc: dmaengine@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com> > --- > drivers/dma/imx-sdma.c | 48 ++++++++++++++++++++++++------------------ > 1 file changed, 27 insertions(+), 21 deletions(-) > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c > index 99d9f431ae2c..3f0f41d16e1c 100644 > --- a/drivers/dma/imx-sdma.c > +++ b/drivers/dma/imx-sdma.c > @@ -2096,27 +2096,6 @@ static int sdma_probe(struct platform_device *pdev) > if (pdata && pdata->script_addrs) > sdma_add_scripts(sdma, pdata->script_addrs); > > - if (pdata) { > - ret = sdma_get_firmware(sdma, pdata->fw_name); > - if (ret) > - dev_warn(&pdev->dev, "failed to get firmware from platform data\n"); > - } else { > - /* > - * Because that device tree does not encode ROM script address, > - * the RAM script in firmware is mandatory for device tree > - * probe, otherwise it fails. > - */ > - ret = of_property_read_string(np, "fsl,sdma-ram-script-name", > - &fw_name); > - if (ret) > - dev_warn(&pdev->dev, "failed to get firmware name\n"); > - else { > - ret = sdma_get_firmware(sdma, fw_name); > - if (ret) > - dev_warn(&pdev->dev, "failed to get firmware from device tree\n"); > - } > - } > - > sdma->dma_device.dev = &pdev->dev; > > sdma->dma_device.device_alloc_chan_resources = sdma_alloc_chan_resources; > @@ -2161,6 +2140,33 @@ static int sdma_probe(struct platform_device *pdev) > of_node_put(spba_bus); > } > > + /* > + * Kick off firmware loading as the very last step: > + * attempt to load firmware only if we're not on the error path, because > + * the firmware callback requires a fully functional and allocated sdma > + * instance. > + */ > + if (pdata) { > + ret = sdma_get_firmware(sdma, pdata->fw_name); > + if (ret) > + dev_warn(&pdev->dev, "failed to get firmware from platform data\n"); > + } else { > + /* > + * Because that device tree does not encode ROM script address, > + * the RAM script in firmware is mandatory for device tree > + * probe, otherwise it fails. > + */ > + ret = of_property_read_string(np, "fsl,sdma-ram-script-name", > + &fw_name); > + if (ret) > + dev_warn(&pdev->dev, "failed to get firmware name\n"); if should have braces! > + else { > + ret = sdma_get_firmware(sdma, fw_name); > + if (ret) > + dev_warn(&pdev->dev, "failed to get firmware from device tree\n"); > + } > + } > + > return 0; > > err_register: > -- > 2.17.1 Applied after fixing braces!
Hi Vinod, On Fri, Jul 5, 2019 at 3:32 AM Vinod Koul <vkoul@kernel.org> wrote: > > > + if (ret) > > + dev_warn(&pdev->dev, "failed to get firmware name\n"); > > if should have braces! > Applied after fixing braces! checkpatch.pl output after adding braces: WARNING: braces {} are not necessary for single statement blocks #102: FILE: drivers/dma/imx-sdma.c:2165: + if (ret) { + dev_warn(&pdev->dev, "failed to get firmware from device tree\n"); + } total: 0 errors, 1 warnings, 61 lines checked
Hi Vinod, On Fri, Jul 5, 2019 at 8:50 AM Vinod Koul <vkoul@kernel.org> wrote: > > there is an else here too! > You are of course right, I was looking at the wrong if. Apologies for the confusion, I did try to look at what you changed, but your git repo listed in MAINTAINERS appears unresponsive for me? Thanks, Sven
On Fri, Jul 5, 2019 at 10:09 AM Sven Van Asbroeck <thesven73@gmail.com> wrote: > You are of course right, I was looking at the wrong if. > > Apologies for the confusion, I did try to look at what you > changed, but your git repo listed in MAINTAINERS appears > unresponsive for me? Works fine here: https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/slave-dma.git/commit/?h=fixes&id=2b8066c3deb9140fdf258417a51479b2aeaa7622
On 05-07-19, 09:08, Sven Van Asbroeck wrote: > Hi Vinod, > > On Fri, Jul 5, 2019 at 8:50 AM Vinod Koul <vkoul@kernel.org> wrote: > > > > there is an else here too! > > > > You are of course right, I was looking at the wrong if. NO issues :) > > Apologies for the confusion, I did try to look at what you > changed, but your git repo listed in MAINTAINERS appears > unresponsive for me? To quote David you need to move to 21st century (like me). IPv4 switch for infradead is down so... meanwhile this would work too: git.kernel.org/pub/scm/linux/kernel/git/vkoul/slave-dma.git
Hi, On Fri, 5 Jul 2019 08:26:12 -0400 Sven Van Asbroeck wrote: > Hi Vinod, > > On Fri, Jul 5, 2019 at 3:32 AM Vinod Koul <vkoul@kernel.org> wrote: > > > > > + if (ret) > > > + dev_warn(&pdev->dev, "failed to get firmware name\n"); > > > > if should have braces! > > Applied after fixing braces! > > checkpatch.pl output after adding braces: > > WARNING: braces {} are not necessary for single statement blocks > #102: FILE: drivers/dma/imx-sdma.c:2165: > + if (ret) { > + dev_warn(&pdev->dev, "failed to get firmware from device tree\n"); > + } > You changed the braces in the wrong place! The comment applied to the previous 'if (ret)' which has an else clause with braces, so the if clause needs braces too. Lothar Waßmann
diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index 99d9f431ae2c..3f0f41d16e1c 100644 --- a/drivers/dma/imx-sdma.c +++ b/drivers/dma/imx-sdma.c @@ -2096,27 +2096,6 @@ static int sdma_probe(struct platform_device *pdev) if (pdata && pdata->script_addrs) sdma_add_scripts(sdma, pdata->script_addrs); - if (pdata) { - ret = sdma_get_firmware(sdma, pdata->fw_name); - if (ret) - dev_warn(&pdev->dev, "failed to get firmware from platform data\n"); - } else { - /* - * Because that device tree does not encode ROM script address, - * the RAM script in firmware is mandatory for device tree - * probe, otherwise it fails. - */ - ret = of_property_read_string(np, "fsl,sdma-ram-script-name", - &fw_name); - if (ret) - dev_warn(&pdev->dev, "failed to get firmware name\n"); - else { - ret = sdma_get_firmware(sdma, fw_name); - if (ret) - dev_warn(&pdev->dev, "failed to get firmware from device tree\n"); - } - } - sdma->dma_device.dev = &pdev->dev; sdma->dma_device.device_alloc_chan_resources = sdma_alloc_chan_resources; @@ -2161,6 +2140,33 @@ static int sdma_probe(struct platform_device *pdev) of_node_put(spba_bus); } + /* + * Kick off firmware loading as the very last step: + * attempt to load firmware only if we're not on the error path, because + * the firmware callback requires a fully functional and allocated sdma + * instance. + */ + if (pdata) { + ret = sdma_get_firmware(sdma, pdata->fw_name); + if (ret) + dev_warn(&pdev->dev, "failed to get firmware from platform data\n"); + } else { + /* + * Because that device tree does not encode ROM script address, + * the RAM script in firmware is mandatory for device tree + * probe, otherwise it fails. + */ + ret = of_property_read_string(np, "fsl,sdma-ram-script-name", + &fw_name); + if (ret) + dev_warn(&pdev->dev, "failed to get firmware name\n"); + else { + ret = sdma_get_firmware(sdma, fw_name); + if (ret) + dev_warn(&pdev->dev, "failed to get firmware from device tree\n"); + } + } + return 0; err_register:
If probe() fails anywhere beyond the point where sdma_get_firmware() is called, then a kernel oops may occur. Problematic sequence of events: 1. probe() calls sdma_get_firmware(), which schedules the firmware callback to run when firmware becomes available, using the sdma instance structure as the context 2. probe() encounters an error, which deallocates the sdma instance structure 3. firmware becomes available, firmware callback is called with deallocated sdma instance structure 4. use after free - kernel oops ! Solution: only attempt to load firmware when we're certain that probe() will succeed. This guarantees that the firmware callback's context will remain valid. Note that the remove() path is unaffected by this issue: the firmware loader will increment the driver module's use count, ensuring that the module cannot be unloaded while the firmware callback is pending or running. To: Robin Gong <yibin.gong@nxp.com> Cc: Vinod Koul <vkoul@kernel.org> Cc: Dan Williams <dan.j.williams@intel.com> Cc: Shawn Guo <shawnguo@kernel.org> Cc: Sascha Hauer <s.hauer@pengutronix.de> Cc: Pengutronix Kernel Team <kernel@pengutronix.de> Cc: Fabio Estevam <festevam@gmail.com> Cc: NXP Linux Team <linux-imx@nxp.com> Cc: dmaengine@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com> --- drivers/dma/imx-sdma.c | 48 ++++++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 21 deletions(-)