[PATCH v5 0/2] irqchip/gic-v3-its: Fix double free on error

[PATCH v5 0/2] irqchip/gic-v3-its: Fix double free on error

[Guanrui Huang]

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

[PATCH v5 1/2] irqchip/gic-v3-its: Fix double free on error

[Guanrui Huang]

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 <maz@kernel.org>
Reviewed-by: Zenghui Yu <yuzenghui@huawei.com>
Signed-off-by: Guanrui Huang <guanrui.huang@linux.alibaba.com>
---
 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

Re: [PATCH v5 1/2] irqchip/gic-v3-its: Fix double free on error

[Markus Elfring]

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

[tip: irq/urgent] irqchip/gic-v3-its: Prevent double free on error

[tip-bot2 for Guanrui Huang]

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 <guanrui.huang@linux.alibaba.com>
AuthorDate:    Thu, 18 Apr 2024 14:10:52 +08:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
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 <guanrui.huang@linux.alibaba.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Marc Zyngier <maz@kernel.org>
Reviewed-by: Zenghui Yu <yuzenghui@huawei.com>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/20240418061053.96803-2-guanrui.huang@linux.alibaba.com
---
 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;
 }

[PATCH v5 2/2] irqchip/gic-v3-its: remove BUG_ON in its_vpe_irq_domain_alloc

[Guanrui Huang]

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 <maz@kernel.org>
Signed-off-by: Guanrui Huang <guanrui.huang@linux.alibaba.com>
---
 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

Re: [PATCH v5 2/2] irqchip/gic-v3-its: remove BUG_ON in its_vpe_irq_domain_alloc

[Zenghui Yu]

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 <maz@kernel.org>
> Signed-off-by: Guanrui Huang <guanrui.huang@linux.alibaba.com>

Reviewed-by: Zenghui Yu <yuzenghui@huawei.com>

[tip: irq/core] irqchip/gic-v3-its: Remove BUG_ON in its_vpe_irq_domain_alloc

[tip-bot2 for Guanrui Huang]

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 <guanrui.huang@linux.alibaba.com>
AuthorDate:    Thu, 18 Apr 2024 14:10:53 +08:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
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 <guanrui.huang@linux.alibaba.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Zenghui Yu <yuzenghui@huawei.com>
Acked-by: Marc Zyngier <maz@kernel.org>
Link: https://lore.kernel.org/r/20240418061053.96803-3-guanrui.huang@linux.alibaba.com

---
 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;

Re: [PATCH v5 0/2] irqchip/gic-v3-its: Fix double free on error

[Marc Zyngier]

On Thu, 18 Apr 2024 07:10:51 +0100,
Guanrui Huang <guanrui.huang@linux.alibaba.com> 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.