linux-kernel - Re: [PATCH] irqdomain: check irq mapping against domain size

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <87y262953h.wl-maz@kernel.org>
Date:   Fri, 05 Nov 2021 12:09:22 +0000
From:   Marc Zyngier <maz@...nel.org>
To:     Ben Dooks <ben.dooks@...ethink.co.uk>
Cc:     linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH] irqdomain: check irq mapping against domain size

Hi Ben,

On Fri, 05 Nov 2021 09:06:01 +0000,
Ben Dooks <ben.dooks@...ethink.co.uk> wrote:
> 
> The irq translate code does not check the irq number against
> the maximum a domain can handle. This can cause an OOPS if
> the firmware data has been damaged in any way. Check the intspec
> or fwdata against the irqdomain and return -EINVAL if over.
> 
> This is the result of bug somewhere in the boot of a SiFive Unmatched
> board where the 5th argument of the pcie node is being damaged which
> causes an OOPS in the startup code.
> 
> Signed-off-by: Ben Dooks <ben.dooks@...ethink.co.uk>
> ---
>  kernel/irq/irqdomain.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 6284443b87ec..e61397420723 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -906,6 +906,8 @@ int irq_domain_xlate_onecell(struct irq_domain *d, struct device_node *ctrlr,
>  {
>  	if (WARN_ON(intsize < 1))
>  		return -EINVAL;
> +	if (WARN_ON(intspec[0] > d->hwirq_max))
> +		return -EINVAL;

This doesn't seem right.

For a start, d->hwirq_max is 0 when the domain is backed by a radix
tree. Also, nothing says that what you read from the DT is something
that should be directly meaningful to the irqdomain. A driver could
well call into this and perform some extra processing on the data
before it lands into the irqdomain.

In general, this looks like DT validation code, and I'm not keen on
that in the core code.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.