linux-kernel - Re: [PATCH] of/irq: Refuse to of_irq_parse_one() more than one IRQ if #interrupt-cells = <0>

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <8bac8f2826f38f1d8f051c85c779482b47f4a388.camel@siemens.com>
Date: Tue, 11 Mar 2025 18:45:53 +0000
From: "Sverdlin, Alexander" <alexander.sverdlin@...mens.com>
To: "robh@...nel.org" <robh@...nel.org>
CC: "saravanak@...gle.com" <saravanak@...gle.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] of/irq: Refuse to of_irq_parse_one() more than one IRQ if
 #interrupt-cells = <0>

Hi Rob!

On Tue, 2025-03-11 at 12:55 -0500, Rob Herring wrote:
> > An #interrupt-cells = <0> property may arguably be a right answer for an
> > interrupt controller having just one interrupt and no options to configure.
> > There are anyway already existing examples in the tree, both in DTs and in
> > the bindings.
> 
> The existing examples are broken and hacks to take advantage of Linux 
> implementation details (IRQCHIP_DECLARE()).
> 
> And #interrupt-cells==0 can't work with 'interrupts'.

I agree.

> > Now the problem is that of_irq_count() called on an interrupt generating
> > device having one of the former controllers as parent would result in an
> > endless loop. It's especially unpleasant in the startup where
> > of_irq_count() <= ... <= of_platform_default_populate_init() will silently
> > hang forever (unless a watchdog bites).
> > 
> > Prevent others from spending the same time on debugging this by refusing to
> > parse more than one IRQ for such controllers.
> 
> I'll happily take a dtschema patch to warn on 0 cells. Then you can find 
> the problem at build time. I generally don't think it's the kernel's job 
> to validate a DT, but if the code can handle something like this then 
> that's good.
> 
> > 
> > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@...mens.com>
> > ---
> >   drivers/of/irq.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> > index 6c843d54ebb11..b3a359c7641d3 100644
> > --- a/drivers/of/irq.c
> > +++ b/drivers/of/irq.c
> > @@ -381,6 +381,13 @@ int of_irq_parse_one(struct device_node *device, int index, struct of_phandle_ar
> >   		goto out;
> >   	}
> >   
> > +	if (!intsize && index) {
> 
> Why are you checking index?

This way of_irq_count() gives "1" on the corresponding nodes.
Do you think that returning "-EINVAL" "if (!intsize)" would
make more sense? I'm concerned about this place because the
code doesn't even crash, but rather hangs until watchdog comes,
which means, there is no backtrace, nothing.

> > +		pr_debug("%pOF trying to map IRQ %d in %pOF having #interrupt-cells = <0>\n",
> > +			 device, index, p);
> > +		res = -EINVAL;
> > +		goto out;
> > +	}
> > +
> >   	pr_debug(" parent=%pOF, intsize=%d\n", p, intsize);
> >   
> >   	/* Copy intspec into irq structure */

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com