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 for Android: free password hash cracker in your pocket

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

Message-ID: <20250311175532.GA3869198-robh@kernel.org>
Date: Tue, 11 Mar 2025 12:55:32 -0500
From: Rob Herring <robh@...nel.org>
To: "A. Sverdlin" <alexander.sverdlin@...mens.com>
Cc: devicetree@...r.kernel.org, Saravana Kannan <saravanak@...gle.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] of/irq: Refuse to of_irq_parse_one() more than one IRQ
 if #interrupt-cells = <0>

On Fri, Mar 07, 2025 at 02:52:18PM +0100, A. Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@...mens.com>
> 
> 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'.

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

> +		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 */
> -- 
> 2.48.1
>