Hello everyone, this is the v5 of the patch to fix double free
in gic driver.
The differences from the v3 and v4:
1. modify description: add "Fixes" in patch 1, as suggested by Markus Elfring.
2. improve patch granularity: split 'remove BUG_ON' into a secord patch,
as suggested by Zenghui and Markus Elfring.
3. modify description: explain why the BUG_ON is useless in patch 2,
as suggested by Marc Zyngier.
Thanks,
Guanrui
Guanrui Huang (2):
irqchip/gic-v3-its: Fix double free on error
irqchip/gic-v3-its: remove BUG_ON in its_vpe_irq_domain_alloc
drivers/irqchip/irq-gic-v3-its.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
--
2.36.1
In its_vpe_irq_domain_alloc, when its_vpe_init() returns an error
with i > 0, its_vpe_irq_domain_free may free bitmap and vprop_page,
and then there is a double free in its_vpe_irq_domain_alloc.
Fix it by calling its_vpe_irq_domain_free directly, bitmap and
vprop_page will be freed in this function.
Fixes: 7d75bbb4bc1a ("irqchip/gic-v3-its: Add VPE irq domain allocation/teardown")
Reviewed-by: Marc Zyngier <[email protected]>
Reviewed-by: Zenghui Yu <[email protected]>
Signed-off-by: Guanrui Huang <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index fca888b36680..2305f6b524a9 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -4561,13 +4561,8 @@ static int its_vpe_irq_domain_alloc(struct irq_domain *domain, unsigned int virq
irqd_set_resend_when_in_progress(irq_get_irq_data(virq + i));
}
- if (err) {
- if (i > 0)
- its_vpe_irq_domain_free(domain, virq, i);
-
- its_lpi_free(bitmap, base, nr_ids);
- its_free_prop_table(vprop_page);
- }
+ if (err)
+ its_vpe_irq_domain_free(domain, virq, i);
return err;
}
--
2.36.1
This BUG_ON() is useless, because the same effect will be obtained
by letting the code run its course and vm being dereferenced,
triggering an exception.
So just remove this check.
Acked-by: Marc Zyngier <[email protected]>
Signed-off-by: Guanrui Huang <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 2305f6b524a9..55c83e19719d 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -4521,8 +4521,6 @@ static int its_vpe_irq_domain_alloc(struct irq_domain *domain, unsigned int virq
struct page *vprop_page;
int base, nr_ids, i, err = 0;
- BUG_ON(!vm);
-
bitmap = its_lpi_alloc(roundup_pow_of_two(nr_irqs), &base, &nr_ids);
if (!bitmap)
return -ENOMEM;
--
2.36.1
On 2024/4/18 14:10, Guanrui Huang wrote:
> This BUG_ON() is useless, because the same effect will be obtained
> by letting the code run its course and vm being dereferenced,
> triggering an exception.
>
> So just remove this check.
>
> Acked-by: Marc Zyngier <[email protected]>
> Signed-off-by: Guanrui Huang <[email protected]>
Reviewed-by: Zenghui Yu <[email protected]>
On Thu, 18 Apr 2024 07:10:51 +0100,
Guanrui Huang <[email protected]> wrote:
>
> Hello everyone, this is the v5 of the patch to fix double free
> in gic driver.
>
> The differences from the v3 and v4:
> 1. modify description: add "Fixes" in patch 1, as suggested by Markus Elfring.
>
> 2. improve patch granularity: split 'remove BUG_ON' into a secord patch,
> as suggested by Zenghui and Markus Elfring.
>
> 3. modify description: explain why the BUG_ON is useless in patch 2,
> as suggested by Marc Zyngier.
>
> Thanks,
> Guanrui
>
> Guanrui Huang (2):
> irqchip/gic-v3-its: Fix double free on error
> irqchip/gic-v3-its: remove BUG_ON in its_vpe_irq_domain_alloc
>
> drivers/irqchip/irq-gic-v3-its.c | 11 ++---------
> 1 file changed, 2 insertions(+), 9 deletions(-)
>
Thomas, can you please take this in for 6.10?
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
I propose to improve the commit message another bit.
How do you think about to append the text “in its_vpe_irq_domain_alloc()”
to the summary phrase?
> In its_vpe_irq_domain_alloc, when its_vpe_init() returns an error
> with i > 0, its_vpe_irq_domain_free may free bitmap and vprop_page,
> and then there is a double free in its_vpe_irq_domain_alloc.
Can it be nicer to avoid the duplicate specification of a function name
in this change description?
> Fix it by calling its_vpe_irq_domain_free directly, bitmap and
> vprop_page will be freed in this function.
* Can the phrase “Fix it by” be omitted for an other imperative wording variant?
* Would you like to separate sentences by a dot instead of combining them
with a comma?
Regards,
Markus
The following commit has been merged into the irq/core branch of tip:
Commit-ID: 382d2ffe86efb1e2fa803d2cf17e5bfc34e574f3
Gitweb: https://git.kernel.org/tip/382d2ffe86efb1e2fa803d2cf17e5bfc34e574f3
Author: Guanrui Huang <[email protected]>
AuthorDate: Thu, 18 Apr 2024 14:10:53 +08:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Thu, 25 Apr 2024 14:38:24 +02:00
irqchip/gic-v3-its: Remove BUG_ON in its_vpe_irq_domain_alloc
This BUG_ON() is useless, because the same effect will be obtained
by letting the code run its course and vm being dereferenced,
triggering an exception.
So just remove this check.
Signed-off-by: Guanrui Huang <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Reviewed-by: Zenghui Yu <[email protected]>
Acked-by: Marc Zyngier <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
drivers/irqchip/irq-gic-v3-its.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 20f9542..98e5593 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -4526,8 +4526,6 @@ static int its_vpe_irq_domain_alloc(struct irq_domain *domain, unsigned int virq
struct page *vprop_page;
int base, nr_ids, i, err = 0;
- BUG_ON(!vm);
-
bitmap = its_lpi_alloc(roundup_pow_of_two(nr_irqs), &base, &nr_ids);
if (!bitmap)
return -ENOMEM;
The following commit has been merged into the irq/urgent branch of tip:
Commit-ID: c26591afd33adce296c022e3480dea4282b7ef91
Gitweb: https://git.kernel.org/tip/c26591afd33adce296c022e3480dea4282b7ef91
Author: Guanrui Huang <[email protected]>
AuthorDate: Thu, 18 Apr 2024 14:10:52 +08:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Thu, 25 Apr 2024 14:30:46 +02:00
irqchip/gic-v3-its: Prevent double free on error
The error handling path in its_vpe_irq_domain_alloc() causes a double free
when its_vpe_init() fails after successfully allocating at least one
interrupt. This happens because its_vpe_irq_domain_free() frees the
interrupts along with the area bitmap and the vprop_page and
its_vpe_irq_domain_alloc() subsequently frees the area bitmap and the
vprop_page again.
Fix this by unconditionally invoking its_vpe_irq_domain_free() which
handles all cases correctly and by removing the bitmap/vprop_page freeing
from its_vpe_irq_domain_alloc().
[ tglx: Massaged change log ]
Fixes: 7d75bbb4bc1a ("irqchip/gic-v3-its: Add VPE irq domain allocation/teardown")
Signed-off-by: Guanrui Huang <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Reviewed-by: Marc Zyngier <[email protected]>
Reviewed-by: Zenghui Yu <[email protected]>
Cc: [email protected]
Link: https://lore.kernel.org/r/[email protected]
---
drivers/irqchip/irq-gic-v3-its.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 2a537cb..5f7d3db 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -4567,13 +4567,8 @@ static int its_vpe_irq_domain_alloc(struct irq_domain *domain, unsigned int virq
irqd_set_resend_when_in_progress(irq_get_irq_data(virq + i));
}
- if (err) {
- if (i > 0)
- its_vpe_irq_domain_free(domain, virq, i);
-
- its_lpi_free(bitmap, base, nr_ids);
- its_free_prop_table(vprop_page);
- }
+ if (err)
+ its_vpe_irq_domain_free(domain, virq, i);
return err;
}