linux-kernel - Re: [PATCH v2] of/irq: Make sure to update out_irq->np to the new parent in of_irq_parse

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: <CAL_Jsq+Wcag2Lzu_kLRb5ia=3hNUOs1Ny93Y541eOY-NZOA5qw@mail.gmail.com>
Date: Thu, 1 Aug 2024 08:49:04 -0600
From: Rob Herring <robh@...nel.org>
To: WHR <whr@...oreo.one>
Cc: Saravana Kannan <saravanak@...gle.com>, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] of/irq: Make sure to update out_irq->np to the new
 parent in of_irq_parse_raw

On Wed, Jul 31, 2024 at 10:22 PM WHR <whr@...oreo.one> wrote:
>
> > On Mon, Jul 29, 2024 at 11:54 PM WHR <whr@...oreo.one> wrote:
> >>
> >> Commit 935df1bd40d43c4ee91838c42a20e9af751885cc has removed an
> >> assignment statement for 'out_irq->np' right after label 'skiplevel',
> >> causing the new parent acquired from function of_irq_find_parent didn't
> >> being stored to 'out_irq->np' as it supposed to. Under some conditions
> >> this can resuit in multiple corruptions and leakages to device nodes.
> >
> > Under what conditions? Please provide a specific platform and DT.
>
> I have a previous email sent to you before I came up with the fix. The kernel
> log for debugging and the device tree blob are attached again.

Thanks. The patch needs to stand on its own with this detail, not
require that I've read (and remember) some other email among the
1000s.

"multiple corruptions and leakages to device nodes" is meaningless. Be
exact, it's device_node refcounts we're talking about. The issue is
out_irq->np is not updated from 'usbdrd' node to the real interrupt
parent, the 'plic' node. In the next iteration of the loop, we find
'interrupt-controller' in the plic node and return, but out_irq is not
pointing to the plic. Then of_irq_get() fails to get the irq host and
does a put on out_irq->np which is usbdrd, not plic node.

So please update the commit msg and provide your name, not initials.

>
> > Honestly, I think the DT is wrong if you get to this point. We'd have
> > to have the initial interrupt parent with #interrupt-cells, but not an
> > interrupt-controller nor interrupt-map property to get here. Maybe
> > that happens in some ancient platform, but if so, I want to know which
> > one and what exactly we need to handle.
>
> So you suggest the #interrupt-cells is erroneous in that node, and should be
> removed?

Yes. dtc warns about this. dtschema would too if there was a schema
(there is, but not since you use a downstream binding).

The clint node has the same issue.

> This is a device vendor-provided DT, so any issue in it will have to be fixed
> locally.

Complain to your vendor...

Rob