From: Vinod Koul <vkoul@kernel.org>
To: Sven Van Asbroeck <thesven73@gmail.com>
Cc: Robin Gong <yibin.gong@nxp.com>,
Dan Williams <dan.j.williams@intel.com>,
Shawn Guo <shawnguo@kernel.org>,
Sascha Hauer <s.hauer@pengutronix.de>,
Pengutronix Kernel Team <kernel@pengutronix.de>,
Fabio Estevam <festevam@gmail.com>,
NXP Linux Team <linux-imx@nxp.com>,
dmaengine@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] dmaengine: imx-sdma: fix use-after-free on probe error path
Date: Fri, 5 Jul 2019 12:58:47 +0530 [thread overview]
Message-ID: <20190705072847.GA2911@vkoul-mobl> (raw)
In-Reply-To: <20190624140731.24080-1-TheSven73@gmail.com>
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!
--
~Vinod
next prev parent reply other threads:[~2019-07-05 7:32 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-24 14:07 [PATCH] dmaengine: imx-sdma: fix use-after-free on probe error path Sven Van Asbroeck
2019-06-25 7:14 ` Robin Gong
2019-07-05 7:28 ` Vinod Koul [this message]
2019-07-05 12:26 ` Sven Van Asbroeck
2019-07-05 15:47 ` Lothar Waßmann
2019-07-05 12:46 ` Vinod Koul
2019-07-05 13:08 ` Sven Van Asbroeck
2019-07-05 13:10 ` Fabio Estevam
2019-07-05 13:10 ` Vinod Koul
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190705072847.GA2911@vkoul-mobl \
--to=vkoul@kernel.org \
--cc=dan.j.williams@intel.com \
--cc=dmaengine@vger.kernel.org \
--cc=festevam@gmail.com \
--cc=kernel@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-imx@nxp.com \
--cc=linux-kernel@vger.kernel.org \
--cc=s.hauer@pengutronix.de \
--cc=shawnguo@kernel.org \
--cc=thesven73@gmail.com \
--cc=yibin.gong@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).