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 <[email protected]>
Cc: Vinod Koul <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: Sascha Hauer <[email protected]>
Cc: Pengutronix Kernel Team <[email protected]>
Cc: Fabio Estevam <[email protected]>
Cc: NXP Linux Team <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Sven Van Asbroeck <[email protected]>
---
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
Reviewed-by: Robin Gong <[email protected]>
> -----Original Message-----
> From: Sven Van Asbroeck <[email protected]>
> 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 <[email protected]>
> Cc: Vinod Koul <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: Shawn Guo <[email protected]>
> Cc: Sascha Hauer <[email protected]>
> Cc: Pengutronix Kernel Team <[email protected]>
> Cc: Fabio Estevam <[email protected]>
> Cc: NXP Linux Team <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Sven Van Asbroeck <[email protected]>
> ---
> 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 <[email protected]>
> Cc: Vinod Koul <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: Shawn Guo <[email protected]>
> Cc: Sascha Hauer <[email protected]>
> Cc: Pengutronix Kernel Team <[email protected]>
> Cc: Fabio Estevam <[email protected]>
> Cc: NXP Linux Team <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Sven Van Asbroeck <[email protected]>
> ---
> 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
Hi Vinod,
On Fri, Jul 5, 2019 at 3:32 AM Vinod Koul <[email protected]> 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
On 24-06-19, 10:07, Sven Van Asbroeck wrote:
Quoting orignal patch for better context:
> + 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");
> + }
> + }
> +
On 05-07-19, 08:26, Sven Van Asbroeck wrote:
> Hi Vinod,
>
> On Fri, Jul 5, 2019 at 3:32 AM Vinod Koul <[email protected]> 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");
> + }
there is an else here too!
With the patch the checkpatch warns:
CHECK: braces {} should be used on all arms of this statement
#201: FILE: drivers/dma/imx-sdma.c:2161:
+ if (ret)
[...]
+ else {
[...]
Even if the if is a single block and else being multi block, if would
need matching brances. With the change pushed:
total: 0 errors, 0 warnings, 0 checks, 60 lines checked
--
~Vinod
On Fri, Jul 5, 2019 at 10:09 AM Sven Van Asbroeck <[email protected]> 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
Hi Vinod,
On Fri, Jul 5, 2019 at 8:50 AM Vinod Koul <[email protected]> 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 05-07-19, 09:08, Sven Van Asbroeck wrote:
> Hi Vinod,
>
> On Fri, Jul 5, 2019 at 8:50 AM Vinod Koul <[email protected]> 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
--
~Vinod
On Fri, Jul 5, 2019 at 9:13 AM Vinod Koul <[email protected]> wrote:
>
> To quote David you need to move to 21st century (like me).
So true :)
Your MAINTAINERS entry in linux-next is still pointing to
infradead, though. But that's becoming less and less useful,
now that most developers are moving to kernel.org ...
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 <[email protected]> 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