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,
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) {
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.
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.
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; }
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 */
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
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);
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; }
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); }
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);
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
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
linux-stable-mirror@lists.linaro.org
-
Rob Herring -
Zijun Hu