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

[Sudeep Holla]

On Fri, Jul 29, 2022 at 10:18:04AM -0500, Jassi Brar wrote:
> On Fri, Jul 29, 2022 at 6:10 AM Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
> > On Thu, Jul 28, 2022 at 04:07:45PM -0500, Jassi Brar wrote:
> 
> > > >
> > > 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 ?
> >
> I mean what's written.
>

Sorry for asking to be explicit. How can one ensure that doesn't happen
if someone wants to exploit the secure side firmware. That is my concern
and I understand we must block any users of it in the device tree. But
that doesn't prevent people who can run any non-secure side software to
exploit the secure side.

> > 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.
> >
> Why go to those lengths? These are already simpler options available  ;-)
> 1)   while (1) ;      // preferably in some atomic context
> 2)   *((int *) 0) = 0;   // you might want to iterate over offset for
> guaranteed results
> 3) Slightly more work, but you also have the opportunity to erase your
> storage device
>

I know these simple methods but can I hinder secure side services with
these ?

> > > 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@2b1f0000 {
> > >         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";
> > >         #mbox-cells = <1>;
> > >
> > > If this works for you, I could submit a proper patch.
> >
> > No this doesn't work IMO.
> >
> If you _really_ tested and faced an error, please share it.
>

Yes, I did try. But as I hacked the DT to use it(or I can even hack the
kernel if DT is not so easily upgradable). The main point is as the secure
side uses this channel to communicate with SCP for some of the CPU idle
management which is enable by default in the kernel, by just sending the
command using the same secure channel from the Linux I can randomly
ensure the messages sent by secure side is mangled and CPU may either
fail to resume back from suspend or even fail to suspend.

The end result is I just see it just hangs in the non-secure side. While
you could argue it is expected that we should not have used the secure
channel, but I would go further and ask if that needs to be exposed as
there is no way to mark is as disabled or already in use by secure side.

But yes, I see you point as well. Not sure which is better.

> > Yes standalone everything looks fine, but you can
> > insert a module requesting this channel and bring down the system.
> >
> If anyone other than a super-user is able to do that, then you have a
> serious security problem at hand. If you do that as a super-user, have fun.
>

Yes I assuming super user itself. It may not help much to exploit on Juno
much, but will such a mechanism help to exploit and understand the secure
side communication from non-secure side was my general concern as we have
seen and heard of such exploits quite a lot these days. That is the only
reason I was suggesting to not expose any secure mailbox details to the
non-secure world. Juno is probably not a good example platform to make
my point.

I wish there was a way to tell non-secure side that it is disabled for it's
use as it is used by the secure side 🙁.

As Juno platform maintainer, I am not comfortable adding this in case other
platforms copy for it.

--
Regards,
Sudeep