dt-bindings: mailbox: arm,mhu: Make secure interrupt optional
Subsystems (3)
Rob Herring
maintainerPATCH 1/1 dt-bindings: mailbox: arm,mhu: Make secure interrupt optional
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@2b1f0000: interrupts: [[0, 36, 4], [0, 35, 4]] is too short
--- Documentation/devicetree/bindings/mailbox/arm,mhu.yaml | 1 + 1 file changed, 1 insertion(+)
Documentation/devicetree/bindings/mailbox/arm,mhu.yaml +1 -0
@@ -57,6 +57,7 @@ properties: maxItems: 1 interrupts: + minItems: 2 items: - description: low-priority non-secure - description: high-priority non-secure
Signature · 1 line
2.34.1
Jassi Brar
maintainerQuoted 19 lines · Rob Herring
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@2b1f0000: interrupts: [[0, 36, 4], [0, 35, 4]] is too short
Signed-off-by Rob Herring <[email protected]>--- Documentation/devicetree/bindings/mailbox/arm,mhu.yaml | 1 + 1 file changed, 1 insertion(+)Documentation/devicetree/bindings/mailbox/arm,mhu.yaml +0
-0
Documentation/devicetree/bindings/mailbox/arm,mhu.yaml properties: · line 63
Quoted 7 lines
Documentation/devicetree/bindings/mailbox/arm,mhu.yaml +1
-0@@ -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.
thanks
Quoted 3 lines
--
2.34.1
Jassi Brar
maintainerQuoted 22 lines · Jassi Brar
Quoted 20 lines · Rob Herring
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@2b1f0000: interrupts: [[0, 36, 4], [0, 35, 4]] is too short
Signed-off-by Rob Herring <[email protected]>--- Documentation/devicetree/bindings/mailbox/arm,mhu.yaml | 1 + 1 file changed, 1 insertion(+)Documentation/devicetree/bindings/mailbox/arm,mhu.yaml +0
-0@@ -57,6 +57,7 @@ properties: maxItems: 1
Quoted 10 lines
Documentation/devicetree/bindings/mailbox/arm,mhu.yaml properties: · line 63
Quoted 6 lines
Documentation/devicetree/bindings/mailbox/arm,mhu.yaml +1
-0interrupts: + minItems: 2 items: - description: low-priority non-secure - description: high-priority non-secureDo 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.
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.
(quoted diff) +0 -0
b/arch/arm64/boot/dts/arm/juno-base.dtsi
index f6c55877fbd94..004b1566be74d 100644
arch/arm64/boot/dts/arm/juno-base.dtsi +2 -1
@@ -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.
thanks.
Quoted 33 lines · Jassi Brar
Quoted 22 lines · Jassi Brar
Quoted 20 lines · Rob Herring
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@2b1f0000: interrupts: [[0, 36, 4], [0, 35, 4]] is too short
Signed-off-by Rob Herring <[email protected]>--- Documentation/devicetree/bindings/mailbox/arm,mhu.yaml | 1 + 1 file changed, 1 insertion(+)Documentation/devicetree/bindings/mailbox/arm,mhu.yaml +0
-0@@ -57,6 +57,7 @@ properties: maxItems: 1Quoted 10 lines
Documentation/devicetree/bindings/mailbox/arm,mhu.yaml properties: · line 63
Quoted 6 lines
Documentation/devicetree/bindings/mailbox/arm,mhu.yaml +1
-0interrupts: + minItems: 2 items: - description: low-priority non-secure - description: high-priority non-secureDo 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.
Quoted 4 lines
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.
Quoted 5 lines
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.
Quoted 14 lines
index
f6c55877fbd94..004b1566be74d100644arch/arm64/boot/dts/arm/juno-base.dtsi +2
-1@@ -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>;
Quoted 2 lines
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.
Signature · 2 lines
Regards, Sudeep
Jassi Brar
maintainerQuoted 9 lines · Sudeep Holla
Quoted 4 lines · Jassi Brar
Quoted 1 line
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.
Quoted 4 lines
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
Quoted 14 lines
Quoted 14 lines
index
f6c55877fbd94..004b1566be74d100644arch/arm64/boot/dts/arm/juno-base.dtsi +2
-1@@ -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>;
Quoted 4 lines
Quoted 1 line
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.
Quoted 3 lines
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.
cheers.
Quoted 4 lines · Jassi Brar
Quoted 3 lines · Sudeep Holla
Quoted 1 line · Jassi Brar
Quoted 1 line
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
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.
Quoted 11 lines
Quoted 4 lines
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 ?
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
index
f6c55877fbd94..004b1566be74d100644arch/arm64/boot/dts/arm/juno-base.dtsi +2
-1@@ -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>;
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
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.
Quoted 6 lines
Quoted 3 lines
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
Quoted 13 lines · Sudeep Holla
Quoted 10 lines · Jassi Brar
Quoted 4 lines
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
Quoted 3 lines
I know these simple methods but can I hinder secure side services with these ?
Ideally, no. And neither if we enumerate the secure-channel in dt and driver.
See, even if you remove support for the secure channel in the kernel, a doped super-user could always insmod a module that attempts to access the secure address space that you want to "hide".
cheers.
Quoted 14 lines · Jassi Brar
Quoted 13 lines · Sudeep Holla
Quoted 10 lines · Jassi Brar
Quoted 4 lines
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
Quoted 9 lines
Quoted 3 lines
I know these simple methods but can I hinder secure side services with these ?
Ideally, no. And neither if we enumerate the secure-channel in dt and driver.
See, even if you remove support for the secure channel in the kernel, a doped super-user could always insmod a module that attempts to access the secure address space that you want to "hide".
True, generally they should have put this in a separate page/range so TZ could program accordingly and prohibit any access 🙁.
--
Regards,
Sudeep
Rob Herring
maintainerQuoted 56 lines · Sudeep Holla
Quoted 33 lines · Jassi Brar
Quoted 22 lines · Jassi Brar
Quoted 20 lines · Rob Herring
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@2b1f0000: interrupts: [[0, 36, 4], [0, 35, 4]] is too short
Signed-off-by Rob Herring <[email protected]>--- Documentation/devicetree/bindings/mailbox/arm,mhu.yaml | 1 + 1 file changed, 1 insertion(+)Documentation/devicetree/bindings/mailbox/arm,mhu.yaml +0
-0@@ -57,6 +57,7 @@ properties: maxItems: 1Quoted 10 lines
Documentation/devicetree/bindings/mailbox/arm,mhu.yaml properties: · line 63
Quoted 6 lines
Documentation/devicetree/bindings/mailbox/arm,mhu.yaml +1
-0interrupts: + minItems: 2 items: - description: low-priority non-secure - description: high-priority non-secureDo 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.
Quoted 4 lines
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.
Quoted 5 lines
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.
Quoted 12 lines
arch/arm64/boot/dts/arm/juno-base.dtsi mailbox: mhu@2b1f0000 { · line 32
Quoted 12 lines
index
f6c55877fbd94..004b1566be74d100644arch/arm64/boot/dts/arm/juno-base.dtsi +2
-1@@ -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";
Missing name.
Quoted 8 lines
Quoted 4 lines
arch/arm64/boot/dts/arm/juno-base.dtsi +0
-0#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
Quoted 1 line · Rob Herring
On Fri, Jul 29, 2022 at 5:10 AM Sudeep Holla <[email protected]> wrote:
[...]
Quoted 8 lines
Quoted 4 lines
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 agree, as I mentioned in the other thread, if we had a way to mark that channel as used elsewhere or disabled or unavailable, it would have been great.
Quoted 1 line
I don't really care which way this is fixed though.
Understood.
Quoted 1 line
I just want the warning gone. We've all got better things to worry about.
Agreed.
Quoted 3 lines
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.
I prefer this approach.
Quoted 3 lines
On the flip side, considering it *can* be present already, there's not really much argument for not having it.
Can't disagree/argue that 😄.
--
Regards,
Sudeep
Quoted 7 lines · Rob Herring
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@2b1f0000: interrupts: [[0, 36, 4], [0, 35, 4]] is too short
Signature · 2 lines
Regards, Sudeep