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
From: Bartosz Golaszewski <[email protected]>
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.
--- drivers/gpio/gpio-sim.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
drivers/gpio/gpio-sim.c +16 -0
@@ -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);
Signature · 1 line
2.39.2
PATCH 2/2 gpio: sim: pass the GPIO device's software node to irq domain
From: Bartosz Golaszewski <[email protected]>
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.
--- drivers/gpio/gpio-sim.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpio/gpio-sim.c +1 -1
@@ -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);
Signature · 1 line
2.39.2
Re: [PATCH 2/2] gpio: sim: pass the GPIO device's software node to irq domain
Quoted 5 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
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" ?
Quoted 22 lines
Fixes cb8c474e79be ("gpio: sim: new testing module")Signed-off-by Bartosz Golaszewski <[email protected]>--- drivers/gpio/gpio-sim.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpio/gpio-sim.c +1
-1@@ -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);Signature · 1 line
2.39.2
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 6 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
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.
...
drivers/gpio/gpio-sim.c static void gpio_sim_mutex_destroy(void *data) · line 301
Quoted 9 lines
drivers/gpio/gpio-sim.c +8
-0+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.
Quoted 3 lines
drivers/gpio/gpio-sim.c +3
-0+irq_dispose_mapping (irq); + } +}
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 19 lines · Andy Shevchenko
Quoted 6 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
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....
Quoted 7 lines
drivers/gpio/gpio-sim.c +7
-0+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);
Quoted 4 lines
Quoted 1 line
+ 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
Quoted 9 lines
drivers/gpio/gpio-sim.c static void gpio_sim_mutex_destroy(void *data) · line 304
Quoted 3 lines
drivers/gpio/gpio-sim.c +3
-0+irq_dispose_mapping (irq); + } +}--
With Best Regards,
Andy Shevchenko
Quoted 3 lines · Bartosz Golaszewski
Quoted 1 line · Andy Shevchenko
On Tue, Aug 22, 2023 at 09:51:21AM +0200, Bartosz Golaszewski wrote:
...
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
drivers/gpio/gpio-sim.c +8
-0+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)
Quoted 8 lines
Quoted 2 lines
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.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/gpio/gpio-sim.c +3
-0+irq_dispose_mapping (irq); + } +}
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 27 lines · Andy Shevchenko
Quoted 3 lines · Bartosz Golaszewski
Quoted 1 line · Andy Shevchenko
On Tue, Aug 22, 2023 at 09:51:21AM +0200, Bartosz Golaszewski wrote:
...
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
drivers/gpio/gpio-sim.c +8
-0+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)Quoted 8 lines
Quoted 2 lines
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));?
Quoted 9 lines
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
Quoted 9 lines
Quoted 3 lines
Quoted 3 lines
drivers/gpio/gpio-sim.c static void gpio_sim_mutex_destroy(void *data) · line 304
Quoted 3 lines
drivers/gpio/gpio-sim.c +3
-0+irq_dispose_mapping (irq); + } +}--
With Best Regards,
Andy Shevchenko
Quoted 6 lines · Bartosz Golaszewski
Quoted 4 lines · Andy Shevchenko
Quoted 3 lines · Bartosz Golaszewski
Quoted 1 line · Andy Shevchenko
On Tue, Aug 22, 2023 at 09:51:21AM +0200, Bartosz Golaszewski wrote:
...
Quoted 25 lines
Quoted 25 lines
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
drivers/gpio/gpio-sim.c +8
-0+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)Quoted 8 lines
Quoted 2 lines
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.
Quoted 6 lines
Quoted 3 lines
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.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/gpio/gpio-sim.c +3
-0+irq_dispose_mapping (irq); + } +}
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 7 lines · Andy Shevchenko
Quoted 6 lines · Bartosz Golaszewski
Quoted 4 lines · Andy Shevchenko
Quoted 3 lines · Bartosz Golaszewski
Quoted 1 line · Andy Shevchenko
On Tue, Aug 22, 2023 at 09:51:21AM +0200, Bartosz Golaszewski wrote:
...
Quoted 25 lines
Quoted 25 lines
Quoted 25 lines
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
drivers/gpio/gpio-sim.c +8
-0+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)Quoted 8 lines
Quoted 2 lines
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.
Quoted 12 lines
Quoted 6 lines
Quoted 3 lines
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.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/gpio/gpio-sim.c +3
-0+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.
Signature · 2 lines
With Best Regards, Andy Shevchenko