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

risc-v/mpfs: ddr: lock segmentation registers #15647

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

eenurkka
Copy link
Contributor

Set the LOCKED bit when the final ddr segmentatition is in place. Otherwise the system is prone to potential security issues if the config is altered later. Once the LOCKED bit is set, the register may no longer be changed.

Summary

DDR segmentations registers should be locked after DDR training. Otherwise they may be altered later, leading to potential security issues.

Impact

All mpfs and derived products.

Testing

This has been tested and used on multiple devices, including Polarfire Icicle.

Set the LOCKED bit when the final ddr segmentatition is
in place. Otherwise the system is prone to potential
security issues if the config is altered later. Once the
LOCKED bit is set, the register may no longer be changed.

Signed-off-by: Eero Nurkkala <[email protected]>
@github-actions github-actions bot added Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Size: S The size of the change in this PR is small labels Jan 22, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 22, 2025

[Experimental Bot, please feedback here]

No, this PR description does not fully meet the NuttX requirements. Here's why and how to fix it:

Missing/Insufficient Information:

  • Summary: Lacks specifics. Which DDR segmentation registers? How does the locking mechanism work? What code is changed to implement the lock?
  • Impact: "All mpfs and derived products" is vague. List specific architectures/boards. Needs explicit YES/NO answers for all impact categories, followed by descriptions if YES. This includes documentation (does it need updating to describe the new lock feature?), security (explicitly state how this mitigates security issues), compatibility (are older configurations still supported?), etc.
  • Testing: "Tested on multiple devices" is insufficient. Provide specific build host and target details as requested (OS, CPU, compiler, architecture, board, configuration). The "testing logs" sections are empty. Provide actual logs or commands used for testing, demonstrating the issue before the change and the successful fix after the change. Show how you verified that the registers are locked and cannot be altered.

How to Improve the PR Description:

## Summary

This PR locks the DDR segmentation registers (specifically registers X, Y, and Z - replace with actual register names) after DDR training is complete to prevent subsequent modification and potential security vulnerabilities. The change adds a call to the new function `ddr_lock_segmentation()` within the `ddr_initialize()` function after training completes. This function sets the LOCK bit in the control register (register A - replace with actual register name), making further changes to the segmentation registers impossible.  Fixes potential security vulnerability where malicious actors could modify DDR segmentation after initialization.

Related [NuttX Issue](https://github.com/apache/nuttx/issues/XXXX) - if applicable

## Impact

* Is new feature added? NO (mitigation considered a bug fix)
* Is existing feature changed? YES (DDR initialization now includes a locking step)
* Impact on user (will user need to adapt to change)? NO
* Impact on build (will build process change)? NO
* Impact on hardware (will arch(s) / board(s) / driver(s) change)? YES (Affects Microchip PolarFire SoC MPFS and Icicle Kit. May affect other MPFS-based boards.)
* Impact on documentation (is update required / provided)? YES (The documentation will be updated to describe the new locking mechanism and its security implications.)
* Impact on security (any sort of implications)? YES (This change mitigates a potential security vulnerability where unauthorized modification of DDR segmentation could lead to system instability or information leakage.)
* Impact on compatibility (backward/forward/interoperability)? NO
* Anything else to consider?  N/A


## Testing

I confirm that changes are verified on local setup and works as intended:
* Build Host(s): Linux (Ubuntu 20.04), x86_64, GCC 9.4.0
* Target(s): RISC-V, Microchip PolarFire SoC Icicle Kit (mpfs_icicle_defconfig)

Testing logs before change:

Demonstrate how you attempted to change the DDR segmentation registers after initialization and observed that it was possible.

cat /proc/ddr_segmentation # Show current values
echo "new_value" > /proc/ddr_segmentation/register_X # Example - show attempt to change a register
cat /proc/ddr_segmentation # Show that the value changed successfully, demonstrating the vulnerability


Testing logs after change:

Demonstrate that attempting to change the registers after initialization now fails.

cat /proc/ddr_segmentation # Show current values
echo "new_value" > /proc/ddr_segmentation/register_X # Example - attempt to change a register
cat /proc/ddr_segmentation # Show that the value did not change, and ideally, show an error message indicating the registers are locked.

Also verify the LOCK bit is set in the control register (e.g., using a register dump tool).

This improved description provides more detail, specifically addressing each requirement of the NuttX PR template. Remember to replace the placeholder register names and commands with your actual values and adapt the testing logs to accurately reflect your test setup and results. A clear and comprehensive PR description is crucial for efficient review and integration.

@xiaoxiang781216 xiaoxiang781216 merged commit 9424abf into apache:master Jan 22, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants