Skip to contentSkip to conversation

moxart: fix potential use-after-free on remove path

PATCH Superseded Greg Kroah-Hartman 2 participants 5 messages Jan 14, 2022 – Jan 27, 2022

Subsystems (2)
Stable branch MMC, SD and SDIO

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Greg Kroah-Hartman

author maintainer
#0 lore raw

PATCH 1/1 moxart: fix potential use-after-free on remove path

It was reported that the mmc host structure could be accessed after it was freed in moxart_remove(), so fix this by saving the base register of the device and using it instead of the pointer dereference.

Cc Ulf Hansson <[email protected]>
Cc Xiyu Yang <[email protected]>
Cc Xin Xiong <[email protected]>
Cc Xin Tan <[email protected]>
Cc Tony Lindgren <[email protected]>
Cc Yang Li <[email protected]>
Cc [email protected]
Cc stable <[email protected]>
Reported-by whitehat002 <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 drivers/mmc/host/moxart-mmc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/mmc/host/moxart-mmc.c +5 -4
index 16d1c7a43d33..f5d96940a9b8 100644
@@ -697,6 +697,7 @@ static int moxart_remove(struct platform_device *pdev)
 {
 	struct mmc_host *mmc = dev_get_drvdata(&pdev->dev);
 	struct moxart_host *host = mmc_priv(mmc);
+	void __iomem *base = host->base;
 
 	dev_set_drvdata(&pdev->dev, NULL);
 

@@ -707,10 +708,10 @@ static int moxart_remove(struct platform_device *pdev)
 	mmc_remove_host(mmc);
 	mmc_free_host(mmc);
 
-	writel(0, host->base + REG_INTERRUPT_MASK);
-	writel(0, host->base + REG_POWER_CONTROL);
-	writel(readl(host->base + REG_CLOCK_CONTROL) | CLK_OFF,
-	       host->base + REG_CLOCK_CONTROL);
+	writel(0, base + REG_INTERRUPT_MASK);
+	writel(0, base + REG_POWER_CONTROL);
+	writel(readl(base + REG_CLOCK_CONTROL) | CLK_OFF,
+	       base + REG_CLOCK_CONTROL);
 
 	return 0;
 }
Signature · 1 line
2.34.1
#1 lore raw
Quoted 32 lines · Greg Kroah-Hartman

It was reported that the mmc host structure could be accessed after it was freed in moxart_remove(), so fix this by saving the base register of the device and using it instead of the pointer dereference.

Cc Ulf Hansson <[email protected]>
Cc Xiyu Yang <[email protected]>
Cc Xin Xiong <[email protected]>
Cc Xin Tan <[email protected]>
Cc Tony Lindgren <[email protected]>
Cc Yang Li <[email protected]>
Cc [email protected]
Cc stable <[email protected]>
Reported-by whitehat002 <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 drivers/mmc/host/moxart-mmc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/mmc/host/moxart-mmc.c +1 -0
index 16d1c7a43d33..f5d96940a9b8 100644
@@ -697,6 +697,7 @@ static int moxart_remove(struct platform_device *pdev)
 {
        struct mmc_host *mmc = dev_get_drvdata(&pdev->dev);
        struct moxart_host *host = mmc_priv(mmc);
+       void __iomem *base = host->base;
 
        dev_set_drvdata(&pdev->dev, NULL);

drivers/mmc/host/moxart-mmc.c static int moxart_remove(struct platform_device *pdev) · line 713

Quoted 7 lines
drivers/mmc/host/moxart-mmc.c +0 -4
@@ -707,10 +708,10 @@ static int moxart_remove(struct platform_device *pdev)
        mmc_remove_host(mmc);
        mmc_free_host(mmc);
 
-       writel(0, host->base + REG_INTERRUPT_MASK);
-       writel(0, host->base + REG_POWER_CONTROL);
-       writel(readl(host->base + REG_CLOCK_CONTROL) | CLK_OFF,
-              host->base + REG_CLOCK_CONTROL);

Rather than doing it like this, I think it would be easier to move mmc_free_host() below this part. That's usually what mmc host drivers do clean up things in ->remove().

drivers/mmc/host/moxart-mmc.c static int moxart_remove(struct platform_device *pdev) · line 717

Quoted 7 lines
drivers/mmc/host/moxart-mmc.c +4 -0
+       writel(0, base + REG_INTERRUPT_MASK);
+       writel(0, base + REG_POWER_CONTROL);
+       writel(readl(base + REG_CLOCK_CONTROL) | CLK_OFF,
+              base + REG_CLOCK_CONTROL);
 
        return 0;
 }

Kind regards
Uffe

author maintainer
#2 lore raw
Quoted 37 lines · Ulf Hansson
Quoted 35 lines · Greg Kroah-Hartman

It was reported that the mmc host structure could be accessed after it was freed in moxart_remove(), so fix this by saving the base register of the device and using it instead of the pointer dereference.

Cc Ulf Hansson <[email protected]>
Cc Xiyu Yang <[email protected]>
Cc Xin Xiong <[email protected]>
Cc Xin Tan <[email protected]>
Cc Tony Lindgren <[email protected]>
Cc Yang Li <[email protected]>
Cc [email protected]
Cc stable <[email protected]>
Reported-by whitehat002 <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 drivers/mmc/host/moxart-mmc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/mmc/host/moxart-mmc.c +1 -0
index 16d1c7a43d33..f5d96940a9b8 100644
@@ -697,6 +697,7 @@ static int moxart_remove(struct platform_device *pdev)
 {
        struct mmc_host *mmc = dev_get_drvdata(&pdev->dev);
        struct moxart_host *host = mmc_priv(mmc);
+       void __iomem *base = host->base;
 
        dev_set_drvdata(&pdev->dev, NULL);
 

@@ -707,10 +708,10 @@ static int moxart_remove(struct platform_device *pdev)
        mmc_remove_host(mmc);
        mmc_free_host(mmc);
Quoted 14 lines

drivers/mmc/host/moxart-mmc.c static int moxart_remove(struct platform_device *pdev) · line 713

Quoted 4 lines
drivers/mmc/host/moxart-mmc.c +0 -4
-       writel(0, host->base + REG_INTERRUPT_MASK);
-       writel(0, host->base + REG_POWER_CONTROL);
-       writel(readl(host->base + REG_CLOCK_CONTROL) | CLK_OFF,
-              host->base + REG_CLOCK_CONTROL);

Rather than doing it like this, I think it would be easier to move mmc_free_host() below this part. That's usually what mmc host drivers do clean up things in ->remove().

Quoted 5 lines
drivers/mmc/host/moxart-mmc.c +4 -0
+       writel(0, base + REG_INTERRUPT_MASK);
+       writel(0, base + REG_POWER_CONTROL);
+       writel(readl(base + REG_CLOCK_CONTROL) | CLK_OFF,
+              base + REG_CLOCK_CONTROL);

Ok, I can do that, I didn't know if it would cause any functionality changes, so I was trying to preserve the same logic that the driver currently has.

Do you have this device to test this with?

thanks,

greg k-h

#3 lore raw
Quoted 54 lines · Greg Kroah-Hartman
Quoted 37 lines · Ulf Hansson
Quoted 35 lines · Greg Kroah-Hartman

It was reported that the mmc host structure could be accessed after it was freed in moxart_remove(), so fix this by saving the base register of the device and using it instead of the pointer dereference.

Cc Ulf Hansson <[email protected]>
Cc Xiyu Yang <[email protected]>
Cc Xin Xiong <[email protected]>
Cc Xin Tan <[email protected]>
Cc Tony Lindgren <[email protected]>
Cc Yang Li <[email protected]>
Cc [email protected]
Cc stable <[email protected]>
Reported-by whitehat002 <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 drivers/mmc/host/moxart-mmc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/mmc/host/moxart-mmc.c +1 -0
index 16d1c7a43d33..f5d96940a9b8 100644
@@ -697,6 +697,7 @@ static int moxart_remove(struct platform_device *pdev)
 {
        struct mmc_host *mmc = dev_get_drvdata(&pdev->dev);
        struct moxart_host *host = mmc_priv(mmc);
+       void __iomem *base = host->base;
 
        dev_set_drvdata(&pdev->dev, NULL);
 

@@ -707,10 +708,10 @@ static int moxart_remove(struct platform_device *pdev)
        mmc_remove_host(mmc);
        mmc_free_host(mmc);
Quoted 14 lines

drivers/mmc/host/moxart-mmc.c static int moxart_remove(struct platform_device *pdev) · line 713

Quoted 4 lines
drivers/mmc/host/moxart-mmc.c +0 -4
-       writel(0, host->base + REG_INTERRUPT_MASK);
-       writel(0, host->base + REG_POWER_CONTROL);
-       writel(readl(host->base + REG_CLOCK_CONTROL) | CLK_OFF,
-              host->base + REG_CLOCK_CONTROL);

Rather than doing it like this, I think it would be easier to move mmc_free_host() below this part. That's usually what mmc host drivers do clean up things in ->remove().

Quoted 5 lines
drivers/mmc/host/moxart-mmc.c +4 -0
+       writel(0, base + REG_INTERRUPT_MASK);
+       writel(0, base + REG_POWER_CONTROL);
+       writel(readl(base + REG_CLOCK_CONTROL) | CLK_OFF,
+              base + REG_CLOCK_CONTROL);
Quoted 3 lines

Ok, I can do that, I didn't know if it would cause any functionality changes, so I was trying to preserve the same logic that the driver currently has.

Yes, but it's most likely just a simple mistake that was made by the original author.

Quoted 2 lines

Do you have this device to test this with?

No, I don't.

Although, I am confident that it should work fine too.

Quoted 4 lines

thanks,

greg k-h

Kind regards
Uffe

author maintainer
#4 lore raw
Quoted 64 lines · Ulf Hansson
Quoted 54 lines · Greg Kroah-Hartman
Quoted 37 lines · Ulf Hansson
Quoted 35 lines · Greg Kroah-Hartman

It was reported that the mmc host structure could be accessed after it was freed in moxart_remove(), so fix this by saving the base register of the device and using it instead of the pointer dereference.

Cc Ulf Hansson <[email protected]>
Cc Xiyu Yang <[email protected]>
Cc Xin Xiong <[email protected]>
Cc Xin Tan <[email protected]>
Cc Tony Lindgren <[email protected]>
Cc Yang Li <[email protected]>
Cc [email protected]
Cc stable <[email protected]>
Reported-by whitehat002 <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 drivers/mmc/host/moxart-mmc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/mmc/host/moxart-mmc.c +1 -0
index 16d1c7a43d33..f5d96940a9b8 100644
@@ -697,6 +697,7 @@ static int moxart_remove(struct platform_device *pdev)
 {
        struct mmc_host *mmc = dev_get_drvdata(&pdev->dev);
        struct moxart_host *host = mmc_priv(mmc);
+       void __iomem *base = host->base;
 
        dev_set_drvdata(&pdev->dev, NULL);
 

@@ -707,10 +708,10 @@ static int moxart_remove(struct platform_device *pdev)
        mmc_remove_host(mmc);
        mmc_free_host(mmc);
Quoted 14 lines

drivers/mmc/host/moxart-mmc.c static int moxart_remove(struct platform_device *pdev) · line 713

Quoted 4 lines
drivers/mmc/host/moxart-mmc.c +0 -4
-       writel(0, host->base + REG_INTERRUPT_MASK);
-       writel(0, host->base + REG_POWER_CONTROL);
-       writel(readl(host->base + REG_CLOCK_CONTROL) | CLK_OFF,
-              host->base + REG_CLOCK_CONTROL);

Rather than doing it like this, I think it would be easier to move mmc_free_host() below this part. That's usually what mmc host drivers do clean up things in ->remove().

Quoted 5 lines
drivers/mmc/host/moxart-mmc.c +4 -0
+       writel(0, base + REG_INTERRUPT_MASK);
+       writel(0, base + REG_POWER_CONTROL);
+       writel(readl(base + REG_CLOCK_CONTROL) | CLK_OFF,
+              base + REG_CLOCK_CONTROL);
Quoted 3 lines

Ok, I can do that, I didn't know if it would cause any functionality changes, so I was trying to preserve the same logic that the driver currently has.

Yes, but it's most likely just a simple mistake that was made by the original author.

Quoted 1 line
Quoted 5 lines
Quoted 1 line

Do you have this device to test this with?

No, I don't.

Although, I am confident that it should work fine too.

Great, now sent v2:

	https://lore.kernel.org/r/[email protected]