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

From: Baruch Siach <baruch@tkos.co.il>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"Thomas Gleixner" <tglx@linutronix.de>
Cc: Thierry Reding <thierry.reding@gmail.com>,
	Lee Jones <lee.jones@linaro.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Gregory Clement <gregory.clement@bootlin.com>,
	Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Chris Packham <chris.packham@alliedtelesis.co.nz>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Ralph Sennhauser <ralph.sennhauser@gmail.com>,
	linux-pwm@vger.kernel.org, linux-gpio@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2] gpio: mvebu: fix potential user-after-free on probe
Date: Mon, 30 Nov 2020 20:12:53 +0200	[thread overview]
Message-ID: <878saipvbu.fsf@tarshish> (raw)
In-Reply-To: <20201130153036.p3gdsauxsmas3rbo@pengutronix.de>

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 -