linux-kernel - Re: [PATCH] dt-bindings: mailbox: arm,mhu: Make secure interrupt optional

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_JsqLwk+=CeDWqXzhrVESzhJnNjOjyR-TtisKGgjMUi7TivQ@mail.gmail.com>
Date:   Fri, 29 Jul 2022 17:17:26 -0600
From:   Rob Herring <robh@...nel.org>
To:     Sudeep Holla <sudeep.holla@....com>
Cc:     Jassi Brar <jassisinghbrar@...il.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Jassi Brar <jaswinder.singh@...aro.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        devicetree@...r.kernel.org
Subject: Re: [PATCH] dt-bindings: mailbox: arm,mhu: Make secure interrupt optional

On Fri, Jul 29, 2022 at 5:10 AM Sudeep Holla <sudeep.holla@....com> wrote:
>
> On Thu, Jul 28, 2022 at 04:07:45PM -0500, Jassi Brar wrote:
> > On Thu, Jul 28, 2022 at 3:17 PM Jassi Brar <jassisinghbrar@...il.com> wrote:
> > >
> > > On Thu, Jul 28, 2022 at 2:08 PM Rob Herring <robh@...nel.org> wrote:
> > > >
> > > > The secure interrupt is only useful to secure world, therefore for NS
> > > > users it shouldn't be required. Make it optional.
> > > >
> > > > This fixes a warning on Arm Juno board:
> > > >
> > > > mhu@...f0000: interrupts: [[0, 36, 4], [0, 35, 4]] is too short
> > > >
> > > > Signed-off-by: Rob Herring <robh@...nel.org>
> > > > ---
> > > >  Documentation/devicetree/bindings/mailbox/arm,mhu.yaml | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/mailbox/arm,mhu.yaml b/Documentation/devicetree/bindings/mailbox/arm,mhu.yaml
> > > > index bd49c201477d..d9a4f4a02d7c 100644
> > > > --- a/Documentation/devicetree/bindings/mailbox/arm,mhu.yaml
> > > > +++ b/Documentation/devicetree/bindings/mailbox/arm,mhu.yaml
> > > > @@ -57,6 +57,7 @@ properties:
> > > >      maxItems: 1
> > > >
> > > >    interrupts:
> > > > +    minItems: 2
> > > >      items:
> > > >        - description: low-priority non-secure
> > > >        - description: high-priority non-secure
> > > >
> > > Do we also want to specify that only the secure-irq is optional
> > > because irqs are directly mapped onto channels in the driver, and only
> > > omitting the secure (last) irq will work.
> > >
> > I could learn why specifying secure irq isn't desirable?
> > No non-secure client node would ask for that secure irq/channel, which
> > will simply lay unused.
> >
>
> Did you mean would or wouldn't ?
>
> Anyways I can insert a module that requests this channel and bring down
> the system as accessing anything configure secure from non-secure side
> on Juno results in system hang/error.
>
> > The programming of the secure channel doesn't care if the mode is
> > indeed secure. It all relies on providing accurate information in the
> > device tree :- the controller provides secure and non-secure channels
> > but only non-secure channels are _used_ in non-secure mode.
> >
>
> In general w.r.t the MHU IP may be yes. But if the secure side configures
> it as secure only and claims the channel which is the case on Juno, any
> attempt to access the same from non-secure side is fatal.
>
> > index f6c55877fbd94..004b1566be74d 100644
> > --- a/arch/arm64/boot/dts/arm/juno-base.dtsi
> > +++ b/arch/arm64/boot/dts/arm/juno-base.dtsi
> > @@ -26,7 +26,8 @@ mailbox: mhu@...f0000 {
> >         compatible = "arm,mhu", "arm,primecell";
> >         reg = <0x0 0x2b1f0000 0x0 0x1000>;
> >         interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>,
> > -                <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>;
> > +                <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>,
> > +                <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
> >         interrupt-names = "mhu_lpri_rx",
> >                   "mhu_hpri_rx";

Missing name.

> >         #mbox-cells = <1>;
> >
> > If this works for you, I could submit a proper patch.
> >
>
> No this doesn't work IMO. Yes standalone everything looks fine, but you can
> insert a module requesting this channel and bring down the system. So I am
> not for this change.

Not having the interrupt listed in DT doesn't prevent that. Is this
security by obscurity?

I don't really care which way this is fixed though. I just want the
warning gone. We've all got better things to worry about. The DT not
having the interrupt has been that way for years (presumably) and the
kernel never needs the interrupt, so the schema should reflect
reality. On the flip side, considering it *can* be present already,
there's not really much argument for not having it.

Rob