[PATCH v2] gpio: mvebu: fix potential user-after-free on probe

[PATCH v2] gpio: mvebu: fix potential user-after-free on probe

[Baruch Siach]

When mvebu_pwm_probe() fails IRQ domain is not released. Goto the
err_domain label on failure to release IRQ domain.

Fixes: 757642f9a584 ("gpio: mvebu: Add limited PWM support")
Reported-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
v2: Don't leak pwm resources (Uwe Kleine-König)

This is split out of the "gpio: mvebu: Armada 8K/7K PWM support" series.
I'll rebase the series v2 on top on this fix.
---
 drivers/gpio/gpio-mvebu.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index 433e2c3f3fd5..c53ed975a180 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -1255,8 +1255,11 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
 	}
 
 	/* Some MVEBU SoCs have simple PWM support for GPIO lines */
-	if (IS_ENABLED(CONFIG_PWM))
-		return mvebu_pwm_probe(pdev, mvchip, id);
+	if (IS_ENABLED(CONFIG_PWM)) {
+		err = mvebu_pwm_probe(pdev, mvchip, id);
+		if (err)
+			goto err_domain;
+	}
 
 	return 0;
 
-- 
2.29.2

Re: [PATCH v2] gpio: mvebu: fix potential user-after-free on probe

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1469 bytes --]

On Mon, Nov 30, 2020 at 05:09:53PM +0200, Baruch Siach wrote:
> When mvebu_pwm_probe() fails IRQ domain is not released. Goto the
> err_domain label on failure to release IRQ domain.
> 
> Fixes: 757642f9a584 ("gpio: mvebu: Add limited PWM support")
> Reported-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
> v2: Don't leak pwm resources (Uwe Kleine-König)
> 
> This is split out of the "gpio: mvebu: Armada 8K/7K PWM support" series.
> I'll rebase the series v2 on top on this fix.
> ---
>  drivers/gpio/gpio-mvebu.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> index 433e2c3f3fd5..c53ed975a180 100644
> --- a/drivers/gpio/gpio-mvebu.c
> +++ b/drivers/gpio/gpio-mvebu.c
> @@ -1255,8 +1255,11 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Some MVEBU SoCs have simple PWM support for GPIO lines */
> -	if (IS_ENABLED(CONFIG_PWM))
> -		return mvebu_pwm_probe(pdev, mvchip, id);
> +	if (IS_ENABLED(CONFIG_PWM)) {
> +		err = mvebu_pwm_probe(pdev, mvchip, id);
> +		if (err)
> +			goto err_domain;

I only looked quickly, but I wonder if you need to undo
irq_alloc_domain_generic_chips(), too?!

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] gpio: mvebu: fix potential user-after-free on probe

[Baruch Siach]

Hi Uwe,

(+ tglx)

On Mon, Nov 30 2020, Uwe Kleine-König wrote:
> On Mon, Nov 30, 2020 at 05:09:53PM +0200, Baruch Siach wrote:
>> When mvebu_pwm_probe() fails IRQ domain is not released. Goto the
>> err_domain label on failure to release IRQ domain.
>> 
>> Fixes: 757642f9a584 ("gpio: mvebu: Add limited PWM support")
>> Reported-by: Andrew Lunn <andrew@lunn.ch>
>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>> ---
>> v2: Don't leak pwm resources (Uwe Kleine-König)
>> 
>> This is split out of the "gpio: mvebu: Armada 8K/7K PWM support" series.
>> I'll rebase the series v2 on top on this fix.
>> ---
>>  drivers/gpio/gpio-mvebu.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
>> index 433e2c3f3fd5..c53ed975a180 100644
>> --- a/drivers/gpio/gpio-mvebu.c
>> +++ b/drivers/gpio/gpio-mvebu.c
>> @@ -1255,8 +1255,11 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
>>  	}
>>  
>>  	/* Some MVEBU SoCs have simple PWM support for GPIO lines */
>> -	if (IS_ENABLED(CONFIG_PWM))
>> -		return mvebu_pwm_probe(pdev, mvchip, id);
>> +	if (IS_ENABLED(CONFIG_PWM)) {
>> +		err = mvebu_pwm_probe(pdev, mvchip, id);
>> +		if (err)
>> +			goto err_domain;
>
> I only looked quickly, but I wonder if you need to undo
> irq_alloc_domain_generic_chips(), too?!

So it seems. __irq_alloc_domain_generic_chips() calls kzalloc() for the
gc field of irq_domain. But I could not find any code that releases this
allocation. These drivers call irq_alloc_domain_generic_chips(), but do
not release gc on failure:

drivers/irqchip/irq-ingenic-tcu.c
drivers/irqchip/irq-orion.c
drivers/irqchip/irq-renesas-irqc.c
drivers/irqchip/irq-sunxi-nmi.c
drivers/pinctrl/pinctrl-rockchip.c

Some of them apparently skip the cleanup because the system would be
unusable anyway. But most of them call irq_domain_remove() on failure.

Thomas, what is the right thing to do here? Should we just call

  kfree(mvchip->domain->gc);

directly to release the allocation?

Thanks,
baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -