[PATCH 0/8] of/irq: fix bugs

[PATCH 0/8] of/irq: fix bugs

[Zijun Hu]

This patch series is to fix bugs in drivers/of/irq.c

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
Zijun Hu (8):
      of/irq: Fix wrong value of variable @len in of_irq_parse_imap_parent()
      of/irq: Correct element count for array @dummy_imask in API of_irq_parse_raw()
      of/irq: Fix device node refcount leakage in API of_irq_parse_raw()
      of/irq: Fix using uninitialized variable @addr_len in API of_irq_parse_one()
      of/irq: Fix device node refcount leakage in API of_irq_parse_one()
      of/irq: Fix device node refcount leakages in of_irq_count()
      of/irq: Fix device node refcount leakages in of_irq_init()
      of/irq: Fix device node refcount leakage in API irq_of_parse_and_map()

 drivers/of/irq.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)
---
base-commit: 16ef9c9de0c48b836c5996c6e9792cb4f658c8f1
change-id: 20241208-of_irq_fix-659514bc9aa3

Best regards,
-- 
Zijun Hu <quic_zijuhu@quicinc.com>

[PATCH 1/8] of/irq: Fix wrong value of variable @len in of_irq_parse_imap_parent()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

Fix wrong @len value by 'len--' after 'imap++'
in of_irq_parse_imap_parent().

Fixes: 935df1bd40d4 ("of/irq: Factor out parsing of interrupt-map parent phandle+args from of_irq_parse_raw()")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/irq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 67fc0ceaa5f51c18c14f96f2bb9f82bcb66f890e..43cf60479b9e18eb0eec35f39c147deccd8fe8dd 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -111,6 +111,7 @@ const __be32 *of_irq_parse_imap_parent(const __be32 *imap, int len, struct of_ph
 	else
 		np = of_find_node_by_phandle(be32_to_cpup(imap));
 	imap++;
+	len--;
 
 	/* Check if not found */
 	if (!np) {

-- 
2.34.1

[PATCH 2/8] of/irq: Correct element count for array @dummy_imask in API of_irq_parse_raw()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

Array @dummy_imask only needs MAX_PHANDLE_ARGS elements, but it actually
has (MAX_PHANDLE_ARGS + 1) elements.

Fix by using (MAX_PHANDLE_ARGS - 1) as max element index in initializer.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/irq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 43cf60479b9e18eb0eec35f39c147deccd8fe8dd..758eb9b3714868112e83469d131b244ce77d4e82 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -171,7 +171,7 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
 	struct device_node *ipar, *tnode, *old = NULL;
 	__be32 initial_match_array[MAX_PHANDLE_ARGS];
 	const __be32 *match_array = initial_match_array;
-	const __be32 *tmp, dummy_imask[] = { [0 ... MAX_PHANDLE_ARGS] = cpu_to_be32(~0) };
+	const __be32 *tmp, dummy_imask[] = { [0 ... (MAX_PHANDLE_ARGS - 1)] = cpu_to_be32(~0) };
 	u32 intsize = 1, addrsize;
 	int i, rc = -EINVAL;
 

-- 
2.34.1

[PATCH 3/8] of/irq: Fix device node refcount leakage in API of_irq_parse_raw()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

of_irq_parse_raw() will return when meet condition (@ipar == @newpar)
but Refcount of device node @out_irq->np was increased twice when
directly return there, hence causes @out_irq->np refcount leakage.

Fix by putting @out_irq->np refcount before returning there.

Fixes: 041284181226 ("of/irq: Allow matching of an interrupt-map local to an interrupt controller")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/irq.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 758eb9b3714868112e83469d131b244ce77d4e82..cb39624a5e7799b9d2f4525f42dac4cd921ab403 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -310,6 +310,12 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
 		addrsize = (imap - match_array) - intsize;
 
 		if (ipar == newpar) {
+			/*
+			 * Has got @ipar's refcount, but the refcount was
+			 * got again by of_irq_parse_imap_parent() via its
+			 * alias @newpair.
+			 */
+			of_node_put(ipar);
 			pr_debug("%pOF interrupt-map entry to self\n", ipar);
 			return 0;
 		}

-- 
2.34.1

[PATCH 4/8] of/irq: Fix using uninitialized variable @addr_len in API of_irq_parse_one()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

of_irq_parse_one() may use uninitialized variable @addr_len as shown below:

// @addr_len is uninitialized
int addr_len;

// This operation does not touch @addr_len if it fails.
addr = of_get_property(device, "reg", &addr_len);

// Use uninitialized @addr_len if the operation fails.
if (addr_len > sizeof(addr_buf))
	addr_len = sizeof(addr_buf);

// Check the operation result here.
if (addr)
	memcpy(addr_buf, addr, addr_len);

Fix by initializing @addr_len before the operation.

Fixes: b739dffa5d57 ("of/irq: Prevent device address out-of-bounds read in interrupt map walk")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/irq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index cb39624a5e7799b9d2f4525f42dac4cd921ab403..64c005dfa23bd157d891f3f10526327deb5e2cfa 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -361,6 +361,7 @@ int of_irq_parse_one(struct device_node *device, int index, struct of_phandle_ar
 		return of_irq_parse_oldworld(device, index, out_irq);
 
 	/* Get the reg property (if any) */
+	addr_len = 0;
 	addr = of_get_property(device, "reg", &addr_len);
 
 	/* Prevent out-of-bounds read in case of longer interrupt parent address size */

-- 
2.34.1

[PATCH 5/8] of/irq: Fix device node refcount leakage in API of_irq_parse_one()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

For of_irq_parse_one(), refcount of device node @out_irq->np was got by
successful of_parse_phandle_with_args() invocation, but it does not put
the refcount before return, so causes the device node refcount leakage.

Fix by putting the node's refcount before return as the other branch does.

Fixes: 79d9701559a9 ("of/irq: create interrupts-extended property")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/irq.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 64c005dfa23bd157d891f3f10526327deb5e2cfa..e5a9e24a5bd3606a57a07d0d12d3e774e9c78977 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -373,8 +373,11 @@ int of_irq_parse_one(struct device_node *device, int index, struct of_phandle_ar
 	/* Try the new-style interrupts-extended first */
 	res = of_parse_phandle_with_args(device, "interrupts-extended",
 					"#interrupt-cells", index, out_irq);
-	if (!res)
-		return of_irq_parse_raw(addr_buf, out_irq);
+	if (!res) {
+		p = out_irq->np;
+		res = of_irq_parse_raw(addr_buf, out_irq);
+		goto out;
+	}
 
 	/* Look for the interrupt parent. */
 	p = of_irq_find_parent(device);

-- 
2.34.1

[PATCH 6/8] of/irq: Fix device node refcount leakages in of_irq_count()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

For of_irq_count(), successful of_irq_parse_one() invocation will get
device node @irq.np refcount,

of_irq_count() invokes of_irq_parse_one() to count IRQs, and successful
invocation of the later will get device node @irq.np refcount, but the
former does not put the refcount before next iteration invocation, hence
causes device node refcount leakages.

Fix by putting @irq.np refcount before the next iteration invocation.

Fixes: 3da5278727a8 ("of/irq: Rework of_irq_count()")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/irq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index e5a9e24a5bd3606a57a07d0d12d3e774e9c78977..d917924d80f563e1392cedc616843365bc638f16 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -514,8 +514,10 @@ int of_irq_count(struct device_node *dev)
 	struct of_phandle_args irq;
 	int nr = 0;
 
-	while (of_irq_parse_one(dev, nr, &irq) == 0)
+	while (of_irq_parse_one(dev, nr, &irq) == 0) {
+		of_node_put(irq.np);
 		nr++;
+	}
 
 	return nr;
 }

-- 
2.34.1

[PATCH 7/8] of/irq: Fix device node refcount leakages in of_irq_init()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

of_irq_init() will leak interrupt controller device node refcounts
in two places as explained below:

1) Leak refcounts of both @desc->dev and @desc->interrupt_parent when
   suffers @desc->irq_init_cb() failure.
2) Leak refcount of @desc->interrupt_parent when cleans up list
   @intc_desc_list in the end.

Refcounts of both @desc->dev and @desc->interrupt_parent were got in
the first loop, but of_irq_init() does not put them before kfree(@desc)
in places mentioned above, so causes refcount leakages.

Fix by putting refcounts involved before kfree(@desc).

Fixes: 8363ccb917c6 ("of/irq: add missing of_node_put")
Fixes: c71a54b08201 ("of/irq: introduce of_irq_init")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/irq.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index d917924d80f563e1392cedc616843365bc638f16..29a58d62d97d1ca4d09a4e4d21531b5b9b958494 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -634,6 +634,8 @@ void __init of_irq_init(const struct of_device_id *matches)
 				       __func__, desc->dev, desc->dev,
 				       desc->interrupt_parent);
 				of_node_clear_flag(desc->dev, OF_POPULATED);
+				of_node_put(desc->interrupt_parent);
+				of_node_put(desc->dev);
 				kfree(desc);
 				continue;
 			}
@@ -664,6 +666,7 @@ void __init of_irq_init(const struct of_device_id *matches)
 err:
 	list_for_each_entry_safe(desc, temp_desc, &intc_desc_list, list) {
 		list_del(&desc->list);
+		of_node_put(desc->interrupt_parent);
 		of_node_put(desc->dev);
 		kfree(desc);
 	}

-- 
2.34.1

[PATCH 8/8] of/irq: Fix device node refcount leakage in API irq_of_parse_and_map()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

In irq_of_parse_and_map(), refcount of device node @oirq.np was got
by successful of_irq_parse_one() invocation, but it does not put the
refcount before return, so causes @oirq.np refcount leakage.

Fix by putting @oirq.np refcount before return.

Fixes: e3873444990d ("of/irq: Move irq_of_parse_and_map() to common code")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/irq.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 29a58d62d97d1ca4d09a4e4d21531b5b9b958494..b43c49de935c76cbf1e49391517dd7b1a569b7fa 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -38,11 +38,15 @@
 unsigned int irq_of_parse_and_map(struct device_node *dev, int index)
 {
 	struct of_phandle_args oirq;
+	unsigned int ret;
 
 	if (of_irq_parse_one(dev, index, &oirq))
 		return 0;
 
-	return irq_create_of_mapping(&oirq);
+	ret = irq_create_of_mapping(&oirq);
+	of_node_put(oirq.np);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(irq_of_parse_and_map);
 

-- 
2.34.1

Re: [PATCH 1/8] of/irq: Fix wrong value of variable @len in of_irq_parse_imap_parent()

[Rob Herring]

On Mon, Dec 09, 2024 at 09:24:59PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> Fix wrong @len value by 'len--' after 'imap++'
> in of_irq_parse_imap_parent().
> 
> Fixes: 935df1bd40d4 ("of/irq: Factor out parsing of interrupt-map parent phandle+args from of_irq_parse_raw()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/of/irq.c | 1 +
>  1 file changed, 1 insertion(+)

Applied, but rewrote the commit message:

of/irq: Fix interrupt-map cell length check in of_irq_parse_imap_parent()

On a malformed interrupt-map property which is shorter than expected by 
1 cell, we may read bogus data past the end of the property instead of
returning an error in of_irq_parse_imap_parent().

Decrement the remaining length when skipping over the interrupt parent  
phandle cell.

Re: [PATCH 0/8] of/irq: fix bugs

[Rob Herring]

On Mon, Dec 09, 2024 at 09:24:58PM +0800, Zijun Hu wrote:
> This patch series is to fix bugs in drivers/of/irq.c
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
> Zijun Hu (8):
>       of/irq: Fix wrong value of variable @len in of_irq_parse_imap_parent()
>       of/irq: Correct element count for array @dummy_imask in API of_irq_parse_raw()
>       of/irq: Fix device node refcount leakage in API of_irq_parse_raw()
>       of/irq: Fix using uninitialized variable @addr_len in API of_irq_parse_one()
>       of/irq: Fix device node refcount leakage in API of_irq_parse_one()
>       of/irq: Fix device node refcount leakages in of_irq_count()
>       of/irq: Fix device node refcount leakages in of_irq_init()
>       of/irq: Fix device node refcount leakage in API irq_of_parse_and_map()

How did you find these refcount issues? Can we get a unit test for 
these.

Rob

Re: [PATCH 4/8] of/irq: Fix using uninitialized variable @addr_len in API of_irq_parse_one()

[Rob Herring]

On Mon, Dec 09, 2024 at 09:25:02PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> of_irq_parse_one() may use uninitialized variable @addr_len as shown below:
> 
> // @addr_len is uninitialized
> int addr_len;
> 
> // This operation does not touch @addr_len if it fails.
> addr = of_get_property(device, "reg", &addr_len);
> 
> // Use uninitialized @addr_len if the operation fails.
> if (addr_len > sizeof(addr_buf))
> 	addr_len = sizeof(addr_buf);
> 
> // Check the operation result here.
> if (addr)
> 	memcpy(addr_buf, addr, addr_len);
> 
> Fix by initializing @addr_len before the operation.
> 
> Fixes: b739dffa5d57 ("of/irq: Prevent device address out-of-bounds read in interrupt map walk")
> Cc: stable@vger.kernel.org
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/of/irq.c | 1 +
>  1 file changed, 1 insertion(+)

Applied, thanks.

Rob

Re: [PATCH 1/8] of/irq: Fix wrong value of variable @len in of_irq_parse_imap_parent()

[Zijun Hu]

On 2024/12/10 04:56, Rob Herring wrote:
> Applied, but rewrote the commit message:
> 
> of/irq: Fix interrupt-map cell length check in of_irq_parse_imap_parent()
> 
> On a malformed interrupt-map property which is shorter than expected by 
> 1 cell, we may read bogus data past the end of the property instead of
> returning an error in of_irq_parse_imap_parent().
> 
> Decrement the remaining length when skipping over the interrupt parent  
> phandle cell.

thank you Rob for these good corrections.

Re: [PATCH 0/8] of/irq: fix bugs

[Zijun Hu]

On 2024/12/10 05:15, Rob Herring wrote:
> On Mon, Dec 09, 2024 at 09:24:58PM +0800, Zijun Hu wrote:
>> This patch series is to fix bugs in drivers/of/irq.c
>>
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>> Zijun Hu (8):
>>       of/irq: Fix wrong value of variable @len in of_irq_parse_imap_parent()
>>       of/irq: Correct element count for array @dummy_imask in API of_irq_parse_raw()
>>       of/irq: Fix device node refcount leakage in API of_irq_parse_raw()
>>       of/irq: Fix using uninitialized variable @addr_len in API of_irq_parse_one()
>>       of/irq: Fix device node refcount leakage in API of_irq_parse_one()
>>       of/irq: Fix device node refcount leakages in of_irq_count()
>>       of/irq: Fix device node refcount leakages in of_irq_init()
>>       of/irq: Fix device node refcount leakage in API irq_of_parse_and_map()
> 
> How did you find these refcount issues? Can we get a unit test for 
> these.
>

find them by reading codes.
yes. let me write some necessary unit tests for them.

> Rob

Re: [PATCH 2/8] of/irq: Correct element count for array @dummy_imask in API of_irq_parse_raw()

[Rob Herring]

On Mon, Dec 09, 2024 at 09:25:00PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> Array @dummy_imask only needs MAX_PHANDLE_ARGS elements, but it actually
> has (MAX_PHANDLE_ARGS + 1) elements.
> 
> Fix by using (MAX_PHANDLE_ARGS - 1) as max element index in initializer.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/of/irq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks.

Rob