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

[Baruch Siach <baruch@xxxxxxxxxx>]

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@xxxxxxx>
>> Signed-off-by: Baruch Siach <baruch@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx - tel: +972.52.368.4656, http://www.tkos.co.il -