[PATCH 1/2] gpio: sim: dispose of irq mappings before destroying the irq_sim domain

[PATCH 1/2] gpio: sim: dispose of irq mappings before destroying the irq_sim domain

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

If a GPIO simulator device is unbound with interrupts still requested,
we will hit a use-after-free issue in __irq_domain_deactivate_irq(). The
owner of the irq domain must dispose of all mappings before destroying
the domain object.

Fixes: cb8c474e79be ("gpio: sim: new testing module")
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpio-sim.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
index f1f6f1c32987..27515384aa10 100644
--- a/drivers/gpio/gpio-sim.c
+++ b/drivers/gpio/gpio-sim.c
@@ -291,6 +291,18 @@ static void gpio_sim_mutex_destroy(void *data)
 	mutex_destroy(lock);
 }
 
+static void gpio_sim_dispose_mappings(void *data)
+{
+	struct gpio_sim_chip *chip = data;
+	unsigned int i, irq;
+
+	for (i = 0; i < chip->gc.ngpio; i++) {
+		irq = irq_find_mapping(chip->irq_sim, i);
+		if (irq)
+			irq_dispose_mapping(irq);
+	}
+}
+
 static void gpio_sim_sysfs_remove(void *data)
 {
 	struct gpio_sim_chip *chip = data;
@@ -406,6 +418,10 @@ static int gpio_sim_add_bank(struct fwnode_handle *swnode, struct device *dev)
 	if (IS_ERR(chip->irq_sim))
 		return PTR_ERR(chip->irq_sim);
 
+	ret = devm_add_action_or_reset(dev, gpio_sim_dispose_mappings, chip);
+	if (ret)
+		return ret;
+
 	mutex_init(&chip->lock);
 	ret = devm_add_action_or_reset(dev, gpio_sim_mutex_destroy,
 				       &chip->lock);
-- 
2.39.2

[PATCH 2/2] gpio: sim: pass the GPIO device's software node to irq domain

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Associate the swnode of the GPIO device's (which is the interrupt
controller here) with the irq domain. Otherwise the interrupt-controller
device attribute is will be a no-op.

Fixes: cb8c474e79be ("gpio: sim: new testing module")
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpio-sim.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
index 27515384aa10..835999343f16 100644
--- a/drivers/gpio/gpio-sim.c
+++ b/drivers/gpio/gpio-sim.c
@@ -414,7 +414,7 @@ static int gpio_sim_add_bank(struct fwnode_handle *swnode, struct device *dev)
 	if (!chip->pull_map)
 		return -ENOMEM;
 
-	chip->irq_sim = devm_irq_domain_create_sim(dev, NULL, num_lines);
+	chip->irq_sim = devm_irq_domain_create_sim(dev, swnode, num_lines);
 	if (IS_ERR(chip->irq_sim))
 		return PTR_ERR(chip->irq_sim);
 
-- 
2.39.2

Re: [PATCH 2/2] gpio: sim: pass the GPIO device's software node to irq domain

[Andy Shevchenko]

On Tue, Aug 22, 2023 at 09:51:22AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Associate the swnode of the GPIO device's (which is the interrupt
> controller here) with the irq domain. Otherwise the interrupt-controller
> device attribute is will be a no-op.

"is will be" ?

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Fixes: cb8c474e79be ("gpio: sim: new testing module")
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/gpio/gpio-sim.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
> index 27515384aa10..835999343f16 100644
> --- a/drivers/gpio/gpio-sim.c
> +++ b/drivers/gpio/gpio-sim.c
> @@ -414,7 +414,7 @@ static int gpio_sim_add_bank(struct fwnode_handle *swnode, struct device *dev)
>  	if (!chip->pull_map)
>  		return -ENOMEM;
>  
> -	chip->irq_sim = devm_irq_domain_create_sim(dev, NULL, num_lines);
> +	chip->irq_sim = devm_irq_domain_create_sim(dev, swnode, num_lines);
>  	if (IS_ERR(chip->irq_sim))
>  		return PTR_ERR(chip->irq_sim);
>  
> -- 
> 2.39.2
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] gpio: sim: dispose of irq mappings before destroying the irq_sim domain

[Andy Shevchenko]

On Tue, Aug 22, 2023 at 09:51:21AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> If a GPIO simulator device is unbound with interrupts still requested,
> we will hit a use-after-free issue in __irq_domain_deactivate_irq(). The
> owner of the irq domain must dispose of all mappings before destroying
> the domain object.

...

> +static void gpio_sim_dispose_mappings(void *data)
> +{
> +	struct gpio_sim_chip *chip = data;
> +	unsigned int i, irq;
> +
> +	for (i = 0; i < chip->gc.ngpio; i++) {
> +		irq = irq_find_mapping(chip->irq_sim, i);

> +		if (irq)

This duplicates check in the following call.

> +			irq_dispose_mapping(irq);
> +	}
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] gpio: sim: dispose of irq mappings before destroying the irq_sim domain

[Bartosz Golaszewski]

On Tue, Aug 22, 2023 at 2:12 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Aug 22, 2023 at 09:51:21AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > If a GPIO simulator device is unbound with interrupts still requested,
> > we will hit a use-after-free issue in __irq_domain_deactivate_irq(). The
> > owner of the irq domain must dispose of all mappings before destroying
> > the domain object.
>
> ...
>
> > +static void gpio_sim_dispose_mappings(void *data)
> > +{
> > +     struct gpio_sim_chip *chip = data;
> > +     unsigned int i, irq;
> > +
> > +     for (i = 0; i < chip->gc.ngpio; i++) {
> > +             irq = irq_find_mapping(chip->irq_sim, i);
>
> > +             if (irq)
>
> This duplicates check in the following call.
>

Ah so it can be a direct call:

    irq_dispose_mapping(irq_find_mapping(chip->irq_sim, i));

?

Bart

> > +                     irq_dispose_mapping(irq);
> > +     }
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH 1/2] gpio: sim: dispose of irq mappings before destroying the irq_sim domain

[Andy Shevchenko]

On Tue, Aug 22, 2023 at 02:16:44PM +0200, Bartosz Golaszewski wrote:
> On Tue, Aug 22, 2023 at 2:12 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Aug 22, 2023 at 09:51:21AM +0200, Bartosz Golaszewski wrote:

...

> > > +static void gpio_sim_dispose_mappings(void *data)
> > > +{
> > > +     struct gpio_sim_chip *chip = data;
> > > +     unsigned int i, irq;
> > > +
> > > +     for (i = 0; i < chip->gc.ngpio; i++) {
> > > +             irq = irq_find_mapping(chip->irq_sim, i);
> >
> > > +             if (irq)
> >
> > This duplicates check in the following call.
> >
> 
> Ah so it can be a direct call:
> 
>     irq_dispose_mapping(irq_find_mapping(chip->irq_sim, i));
> 
> ?

Hehe, seems yes and no. According to the code — yes, but code seems buggy,
and compiler may effectively drop the check (haven't checked this, though).

OTOH, the problem may appear if and only if we have no sparse IRQ configuration
which is probably rare.

That said, I will go without check, it's not your issue.
And I found other places in IRQ framework that misses that check.

> > > +                     irq_dispose_mapping(irq);
> > > +     }
> > > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] gpio: sim: dispose of irq mappings before destroying the irq_sim domain

[Bartosz Golaszewski]

On Tue, Aug 22, 2023 at 2:24 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Aug 22, 2023 at 02:16:44PM +0200, Bartosz Golaszewski wrote:
> > On Tue, Aug 22, 2023 at 2:12 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Tue, Aug 22, 2023 at 09:51:21AM +0200, Bartosz Golaszewski wrote:
>
> ...
>
> > > > +static void gpio_sim_dispose_mappings(void *data)
> > > > +{
> > > > +     struct gpio_sim_chip *chip = data;
> > > > +     unsigned int i, irq;
> > > > +
> > > > +     for (i = 0; i < chip->gc.ngpio; i++) {
> > > > +             irq = irq_find_mapping(chip->irq_sim, i);
> > >
> > > > +             if (irq)
> > >
> > > This duplicates check in the following call.
> > >
> >
> > Ah so it can be a direct call:
> >
> >     irq_dispose_mapping(irq_find_mapping(chip->irq_sim, i));
> >
> > ?
>
> Hehe, seems yes and no. According to the code — yes, but code seems buggy,
> and compiler may effectively drop the check (haven't checked this, though).
>
> OTOH, the problem may appear if and only if we have no sparse IRQ configuration
> which is probably rare.
>
> That said, I will go without check, it's not your issue.
> And I found other places in IRQ framework that misses that check.
>

I disagree. If there's no strong contract from the provider of this
function then this check is so cheap that I'm ready to live with it.

Bart

> > > > +                     irq_dispose_mapping(irq);
> > > > +     }
> > > > +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH 1/2] gpio: sim: dispose of irq mappings before destroying the irq_sim domain

[Andy Shevchenko]

On Tue, Aug 22, 2023 at 02:38:28PM +0200, Bartosz Golaszewski wrote:
> On Tue, Aug 22, 2023 at 2:24 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Aug 22, 2023 at 02:16:44PM +0200, Bartosz Golaszewski wrote:
> > > On Tue, Aug 22, 2023 at 2:12 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Tue, Aug 22, 2023 at 09:51:21AM +0200, Bartosz Golaszewski wrote:

...

> > > > > +static void gpio_sim_dispose_mappings(void *data)
> > > > > +{
> > > > > +     struct gpio_sim_chip *chip = data;
> > > > > +     unsigned int i, irq;
> > > > > +
> > > > > +     for (i = 0; i < chip->gc.ngpio; i++) {
> > > > > +             irq = irq_find_mapping(chip->irq_sim, i);
> > > >
> > > > > +             if (irq)
> > > >
> > > > This duplicates check in the following call.
> > > >
> > >
> > > Ah so it can be a direct call:
> > >
> > >     irq_dispose_mapping(irq_find_mapping(chip->irq_sim, i));
> > >
> > > ?
> >
> > Hehe, seems yes and no. According to the code — yes, but code seems buggy,
> > and compiler may effectively drop the check (haven't checked this, though).
> >
> > OTOH, the problem may appear if and only if we have no sparse IRQ configuration
> > which is probably rare.
> >
> > That said, I will go without check, it's not your issue.
> > And I found other places in IRQ framework that misses that check.
> >
> 
> I disagree. If there's no strong contract from the provider of this
> function then this check is so cheap that I'm ready to live with it.

There are plenty of calls that don't check and there are calls that check.

> > > > > +                     irq_dispose_mapping(irq);
> > > > > +     }
> > > > > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] gpio: sim: dispose of irq mappings before destroying the irq_sim domain

[Andy Shevchenko]

On Tue, Aug 22, 2023 at 03:46:38PM +0300, Andy Shevchenko wrote:
> On Tue, Aug 22, 2023 at 02:38:28PM +0200, Bartosz Golaszewski wrote:
> > On Tue, Aug 22, 2023 at 2:24 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Tue, Aug 22, 2023 at 02:16:44PM +0200, Bartosz Golaszewski wrote:
> > > > On Tue, Aug 22, 2023 at 2:12 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Tue, Aug 22, 2023 at 09:51:21AM +0200, Bartosz Golaszewski wrote:

...

> > > > > > +static void gpio_sim_dispose_mappings(void *data)
> > > > > > +{
> > > > > > +     struct gpio_sim_chip *chip = data;
> > > > > > +     unsigned int i, irq;
> > > > > > +
> > > > > > +     for (i = 0; i < chip->gc.ngpio; i++) {
> > > > > > +             irq = irq_find_mapping(chip->irq_sim, i);
> > > > >
> > > > > > +             if (irq)
> > > > >
> > > > > This duplicates check in the following call.
> > > > >
> > > >
> > > > Ah so it can be a direct call:
> > > >
> > > >     irq_dispose_mapping(irq_find_mapping(chip->irq_sim, i));
> > > >
> > > > ?
> > >
> > > Hehe, seems yes and no. According to the code — yes, but code seems buggy,
> > > and compiler may effectively drop the check (haven't checked this, though).
> > >
> > > OTOH, the problem may appear if and only if we have no sparse IRQ configuration
> > > which is probably rare.
> > >
> > > That said, I will go without check, it's not your issue.
> > > And I found other places in IRQ framework that misses that check.
> > >
> > 
> > I disagree. If there's no strong contract from the provider of this
> > function then this check is so cheap that I'm ready to live with it.
> 
> There are plenty of calls that don't check and there are calls that check.
> 
> > > > > > +                     irq_dispose_mapping(irq);
> > > > > > +     }
> > > > > > +}

FWIW, I have checked the assembly and since virq is not a pointer, it has been
checked anyways. Hence I'm 100% sure the dup test is not needed and there is no
bug in the irq_dispose_mapping(). Just a bit hard to read.

-- 
With Best Regards,
Andy Shevchenko