Re: [PATCH v3 0/8] Suspend to RAM support for K3 J7200

2024-01-12 Thread Andrew Davis

On 1/10/24 3:34 AM, Thomas Richard wrote:

On 1/9/24 18:32, Andrew Davis wrote:

On 1/8/24 10:56 AM, Thomas Richard wrote:

This series is the U-Boot part of the work to add the suspend to RAM
support for the K3 J7200 EVM board.

During the boot R5 SPL makes a copy of DM-Firmware and TF-A in memory.
Resume detection is done by reading a magic value in a pmic register
(set by DM-Firmware).

If a resume is detected, R5 SPL run the exit retention sequence of the
DDR. Then it load TF-A and DM-Firmware using the copies done during
the boot
(fit image processing is skipped).
Before to start TF-A, R5 SPL writes a magic value in scratchpad ram. This
will be used by TF-A to detect that the board is resuming.

The copy of TF-A/DM-Firmware, the SPL stack and malloc are located in a
reserved memory region (for the kernel point of view) to avoid any
memory corruption.

This version is mostly to test the firewall protection with the suspend
to ram.


Seems to show the opposite, if you are able to access and load TF-A
back to its spot after resume from un-trusted SPL then the firewall
did not survive suspend to RAM. That is a huge security gap if TIFS
is forgetting to put back the firewalls on resume.. What is the
point of firewalls on these systems if I can just knock them all
out by doing a simple suspend/resume cycle?


Hello Andrew,

Why are you talking about un-trusted SPL, how R5 SPL can be un-trusted ?


Very easily, when was the last time it had a proper security audit,
was it written from the ground up with security in mind, what secure
software development standards does it follow, etc.. No offense to
SPL, it is very good at what it was designed for, but it was not
designed for security like OP-TEE or TF-A.


Our resume sequence starts like a power-on: ROM code is started, it
loads TIFS and R5 SPL.
As defined in the chain of trust [1], ROM authenticates TIFS and R5 SPL.
So I don't understand how R5 SPL can be un-trusted.



You are making a common mistake here, just because some code was
authenticated before it started, doesn't mean it is invulnerable to
attack and cannot be compromised. Authentication only protects
against modification on the boot media.


Then R5 SPL detects that the board is resuming (and does some specific
operations on the DDR).
But even if the board is resuming, TF-A is authenticated (using
ti_secure_image_post_process) like during the boot.


Right, and this is the only thing that saves us here. TF-A being
authenticated by a higher trust entity like TIFS. My comment
was more about the context in DDR as you point out below. I'm
assuming TF-A will gain some way to authenticate that while
loading it in. Right now R5 SPL could read it out or modify
it (and as above, we don't trust R5 SPL), so having that in
plantext and un-firewalled at that point is a problem that
needs fixed for this solution to be complete.

To be fair that is a TF-A problem, but both need to be in place.

Andrew



So once TF-A is loaded, firewall is active.
TF-A restores its context, then jump to its warm entrypoint.

I guess a weak point could be TF-A context (stored in DDR).

[1] https://docs.u-boot.org/en/latest/board/ti/k3.html#chain-of-trust




Some comments (for the v2) were not fixed in this version.


Why not?


This series has been tested with the series [1] to enable the firewall.
At the end of the resume sequence, TF-A is well protected by the
firewall, but OP-TEE remains unprotected.



Then why post this? If it is just to get some eyes on it, then label
it as an "RFC" so our silence isn't considered acceptance, otherwise we
have to manually NAK these each time.


Yes sorry, I totally forgot to label it as RFC.

Regards,

Thomas


Re: [PATCH v3 0/8] Suspend to RAM support for K3 J7200

2024-01-10 Thread Thomas Richard
On 1/9/24 18:32, Andrew Davis wrote:
> On 1/8/24 10:56 AM, Thomas Richard wrote:
>> This series is the U-Boot part of the work to add the suspend to RAM
>> support for the K3 J7200 EVM board.
>>
>> During the boot R5 SPL makes a copy of DM-Firmware and TF-A in memory.
>> Resume detection is done by reading a magic value in a pmic register
>> (set by DM-Firmware).
>>
>> If a resume is detected, R5 SPL run the exit retention sequence of the
>> DDR. Then it load TF-A and DM-Firmware using the copies done during
>> the boot
>> (fit image processing is skipped).
>> Before to start TF-A, R5 SPL writes a magic value in scratchpad ram. This
>> will be used by TF-A to detect that the board is resuming.
>>
>> The copy of TF-A/DM-Firmware, the SPL stack and malloc are located in a
>> reserved memory region (for the kernel point of view) to avoid any
>> memory corruption.
>>
>> This version is mostly to test the firewall protection with the suspend
>> to ram.
> 
> Seems to show the opposite, if you are able to access and load TF-A
> back to its spot after resume from un-trusted SPL then the firewall
> did not survive suspend to RAM. That is a huge security gap if TIFS
> is forgetting to put back the firewalls on resume.. What is the
> point of firewalls on these systems if I can just knock them all
> out by doing a simple suspend/resume cycle?

Hello Andrew,

Why are you talking about un-trusted SPL, how R5 SPL can be un-trusted ?
Our resume sequence starts like a power-on: ROM code is started, it
loads TIFS and R5 SPL.
As defined in the chain of trust [1], ROM authenticates TIFS and R5 SPL.
So I don't understand how R5 SPL can be un-trusted.

Then R5 SPL detects that the board is resuming (and does some specific
operations on the DDR).
But even if the board is resuming, TF-A is authenticated (using
ti_secure_image_post_process) like during the boot.

So once TF-A is loaded, firewall is active.
TF-A restores its context, then jump to its warm entrypoint.

I guess a weak point could be TF-A context (stored in DDR).

[1] https://docs.u-boot.org/en/latest/board/ti/k3.html#chain-of-trust

> 
>> Some comments (for the v2) were not fixed in this version.
> 
> Why not?
> 
>> This series has been tested with the series [1] to enable the firewall.
>> At the end of the resume sequence, TF-A is well protected by the
>> firewall, but OP-TEE remains unprotected.
>>
> 
> Then why post this? If it is just to get some eyes on it, then label
> it as an "RFC" so our silence isn't considered acceptance, otherwise we
> have to manually NAK these each time.

Yes sorry, I totally forgot to label it as RFC.

Regards,

Thomas


Re: [PATCH v3 0/8] Suspend to RAM support for K3 J7200

2024-01-09 Thread Andrew Davis

On 1/8/24 10:56 AM, Thomas Richard wrote:

This series is the U-Boot part of the work to add the suspend to RAM
support for the K3 J7200 EVM board.

During the boot R5 SPL makes a copy of DM-Firmware and TF-A in memory.
Resume detection is done by reading a magic value in a pmic register
(set by DM-Firmware).

If a resume is detected, R5 SPL run the exit retention sequence of the
DDR. Then it load TF-A and DM-Firmware using the copies done during the boot
(fit image processing is skipped).
Before to start TF-A, R5 SPL writes a magic value in scratchpad ram. This
will be used by TF-A to detect that the board is resuming.

The copy of TF-A/DM-Firmware, the SPL stack and malloc are located in a
reserved memory region (for the kernel point of view) to avoid any
memory corruption.

This version is mostly to test the firewall protection with the suspend
to ram.


Seems to show the opposite, if you are able to access and load TF-A
back to its spot after resume from un-trusted SPL then the firewall
did not survive suspend to RAM. That is a huge security gap if TIFS
is forgetting to put back the firewalls on resume.. What is the
point of firewalls on these systems if I can just knock them all
out by doing a simple suspend/resume cycle?


Some comments (for the v2) were not fixed in this version.


Why not?


This series has been tested with the series [1] to enable the firewall.
At the end of the resume sequence, TF-A is well protected by the
firewall, but OP-TEE remains unprotected.



Then why post this? If it is just to get some eyes on it, then label
it as an "RFC" so our silence isn't considered acceptance, otherwise we
have to manually NAK these each time.

Andrew


[1] 
https://lore.kernel.org/all/20231229-binman-firewalling-v7-0-47ed4af30...@ti.com/


Changes in v3:
- At resume, R5 SPL doesn't restore TF-A anymore.
   TF-A is started like during a cold boot. R5 SPL will notify that the board is
   resuming using a magic value written in the scratchpad ram.
   TF-A will restore itself.
- Link to v2:
   
https://lore.kernel.org/u-boot/20231107161802.855154-1-thomas.rich...@bootlin.com/

Changes in v2:
- Set exit retention code for DDR behind CONFIG_K3_J721E_DDRSS
- Check if TF-A is running in DRAM, if yes no need to restore it
- Remove BL31_START macro, and get TF-A start address from the fit image
- Remove the test_enter_suspend command
- Link to v1:
   
https://lore.kernel.org/u-boot/20231016141135.325698-1-thomas.rich...@bootlin.com/

Gowtham Tammana (1):
   DO NOT MERGE: arm: dts: k3-j7200-r5-common: Add pmic node for esm

Gregory CLEMENT (2):
   configs: j7200_evm_r5: Used reserved memory in DDR for stack
   configs: j7200_evm_r5: Move address used for allocation in the
 reserved space

Thomas Richard (5):
   board: ti: j721e: Add resume detection for J7200
   ram: k3-ddrss: Add exit retention support
   board: ti: j721e: Add the missing part of exit retention for k3-ddrss
 (J7200)
   board: ti: j721e: During resume spl notify tf-a that the board is
 resuming
   arm: mach-k3: j7200: Skip fit processing when resuming

  .../arm/dts/k3-j7200-r5-common-proc-board.dts |  17 +-
  arch/arm/mach-k3/common.c |  65 +++-
  .../arm/mach-k3/include/mach/j721e_hardware.h |   1 +
  arch/arm/mach-k3/include/mach/j721e_spl.h |  30 ++
  arch/arm/mach-k3/sysfw-loader.c   |  16 +-
  board/ti/j721e/evm.c  |  77 +
  configs/j7200_evm_r5_defconfig|   4 +-
  drivers/ram/k3-ddrss/k3-ddrss.c   | 307 ++
  8 files changed, 507 insertions(+), 10 deletions(-)



Re: [PATCH v3 0/8] Suspend to RAM support for K3 J7200

2024-01-09 Thread Nishanth Menon
On 17:56-20240108, Thomas Richard wrote:
> This series is the U-Boot part of the work to add the suspend to RAM
> support for the K3 J7200 EVM board.
> 
> During the boot R5 SPL makes a copy of DM-Firmware and TF-A in memory.
> Resume detection is done by reading a magic value in a pmic register
> (set by DM-Firmware).
> 
> If a resume is detected, R5 SPL run the exit retention sequence of the
> DDR. Then it load TF-A and DM-Firmware using the copies done during the boot
> (fit image processing is skipped).
> Before to start TF-A, R5 SPL writes a magic value in scratchpad ram. This
> will be used by TF-A to detect that the board is resuming.
> 
> The copy of TF-A/DM-Firmware, the SPL stack and malloc are located in a
> reserved memory region (for the kernel point of view) to avoid any
> memory corruption.
> 
> This version is mostly to test the firewall protection with the suspend
> to ram.
> Some comments (for the v2) were not fixed in this version.
> This series has been tested with the series [1] to enable the firewall.
> At the end of the resume sequence, TF-A is well protected by the
> firewall, but OP-TEE remains unprotected.
> 
> [1] 
> https://lore.kernel.org/all/20231229-binman-firewalling-v7-0-47ed4af30...@ti.com/
> 
> 

And as usual, as I have already responded on 
https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/25120

My objection to this series mirrors what I have mentioned previously for
TFA as well - I am looking for some common sequence to be defined
between am62x and J7200 family rather than each go completely
tangentially, until that happens, please consider my standing NAK.

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 
849D 1736 249D