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

VisionFive 2 rework loading the main stage via DTFS #720

Merged
merged 2 commits into from
Jan 14, 2024

Conversation

orangecms
Copy link
Contributor

No description provided.

@orangecms orangecms marked this pull request as draft January 13, 2024 17:12
@orangecms
Copy link
Contributor Author

this comes after #716

Copy link

codecov bot commented Jan 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a5f9101) 0.20% compared to head (820b6b2) 0.20%.

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #720   +/-   ##
=====================================
  Coverage   0.20%   0.20%           
=====================================
  Files         23      23           
  Lines        962     962           
=====================================
  Hits           2       2           
  Misses       960     960           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Daniel Maslowski <info@orangecms.org>
Signed-off-by: Daniel Maslowski <info@orangecms.org>
@orangecms orangecms marked this pull request as ready for review January 13, 2024 22:16
@orangecms orangecms enabled auto-merge (rebase) January 13, 2024 22:39
Copy link
Contributor

@rmsyn rmsyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few questions, and suggested SAFETY comments.

let psize = v.unwrap_or(0);
if !found {
if DEBUG {
println!("No main stage yet, inc offset by 0x{psize:x}");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
println!("No main stage yet, inc offset by 0x{psize:x}");
println!("No main stage yet, inc offset by {psize:#x}");

}
(SRAM0_BASE, SRAM0_SIZE) // occupied space
};
let slice = unsafe { core::slice::from_raw_parts(transmute(base), size) };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a SAFETY comment here since we are loading executable memory from FLASH/RAM?


let hart_id = mhartid::read();
// U-Boot proper
fn exec_payload(addr: usize) {
unsafe {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above, maybe add a SAFETY comment about transmuteing executable memory into an EntryPoint?

Since we're not doing a signed/secure boot setup, we're assuming the memory is valid, but I think that should at least be mentioned.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The secure boot topic is a very good point.

If we reverse engineer the previous vendor tool that apparently had some signing capabilities and we figure out how to burn keys into the SoC, then we could offer secure boot; though that is somewhat outside the current scope. :D It would be cool to have though and I'm sure we can do it!

We do jump to code under our own control here, since you would build oreboot fully, and so it is no more or less secure than the rest of the code.

The point on safety - same with the above - is indeed something to take care of. Let's add those notes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re secure boot; see starfive-tech/Tools#1 where I reversed said tool for the simpler header generation that we also have in oreboot

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be cool to have though and I'm sure we can do it!

I think we could just choose an unused offset in the QSPI flash, write a public key there, and load it into memory on boot. Not perfect, since an attacker with write access to the QSPI could rewrite with their own key, but they could also do whatever they want at that point.

Then in the bt0/SPL (or some earlier stage), we load the key + signature, and verify whatever signed pieces of firmware and/or kernel we want.

Copy link
Member

@rminnich rminnich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems good enough to me, but you might want to see about the other comments.

@orangecms orangecms merged commit 31c84d4 into oreboot:main Jan 14, 2024
6 checks passed
@orangecms orangecms deleted the vf2-load-main-rework branch January 14, 2024 22:25
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.

3 participants