Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARMv8.1-M: Add task dedicated PAC key support #1195

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

AhmedIsmail02
Copy link
Contributor

@AhmedIsmail02 AhmedIsmail02 commented Nov 19, 2024

Description

To harden the security, each task is assigned with a dedicated PAC key, so that attackers needs to get the all the PAC keys right to exploit the system using Return Oriented Programming. The kernel is updated to support the following:

  • A PAC key set with a user defined random number generator is generated and is pushed onto the task's stack when a task is created.
  • As part of scheduling, the task's PAC key is stacked/unstacked to/from the task's stack when a task is unscheduled/scheduled from/to run.

Test Steps

Checklist:

  • I have tested my changes. No regression in existing tests.
  • I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

Blocks FreeRTOS/FreeRTOS-Partner-Supported-Demos#19

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@AhmedIsmail02 AhmedIsmail02 requested a review from a team as a code owner November 19, 2024 14:15
Some declarations in portasm.h and FreeRTOS.h will cause
build errors if it's not understood by the assembler.
For example the 'extern' keyword. To avoid these errors
guard any such code inside a #ifdef #endif block so
the code is included in C files but excluded by the
preprocessor in assembly files.

Signed-off-by: Ahmed Ismail <[email protected]>
ARMv8-M TrustZone variant registers stacking
procedure is modified to be consistent with the NTZ port
variant where one `stmdb` instruction is used instead of
using 'subs' instruction along with `stmia` instruction, also,
this result in more efficient context switching
handling (lower latency).

Signed-off-by: Ahmed Ismail <[email protected]>
To harden the security, each task is assigned
a dedicated PAC key, so that attackers needs
to guess the all the tasks' PAC keys right to
exploit the system using Return Oriented Programming.

The kernel is now updated to support the following:
* A PAC key set with a random number generated and
is pushed onto the task's stack when a task is created.

* As part of scheduling, the task's PAC key is stacked/unstacked
to/from the task's stack when a task is unscheduled/scheduled
from/to run.

Signed-off-by: Ahmed Ismail <[email protected]>
@AhmedIsmail02 AhmedIsmail02 force-pushed the add-task-dedicated-pac-key-support branch from d7e9592 to 05a1578 Compare November 19, 2024 16:39
@aggarg
Copy link
Member

aggarg commented Dec 18, 2024

Thank you for your contribution @AhmedIsmail02! I have reviewed the code and I have the following suggestions -

  1. We should make the RandomKeyGeneration a mandatory function for the application to provide. It has the following benefits:
    a. The application writer need to make an explicit choice if they want to use cryptographically weak random generator (like srand) rather than accidentally defaulting to one.
    b. It is consistent with heap protector - https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/portable/MemMang/heap_4.c#L117.
    c. We do not need to take a dependency on time.h in the port files.
  2. I am not sure if we really need all those __ICCARM__ guards. I removed all those and I am still able to build and run on the code on Cortex-M33 using IAR.
  3. One code path missed saving and restoring PAC keys in the Cortex-M33 PendSV Handler.

All of my suggestions are in the following patch - 0001-Code-review-suggestions.patch. Please take a look and let me know what you think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants