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

Code rebase onto the most recent work implementing Secure Launch protocol being upstreamed to Linux and GRUB #17

Closed
BeataZdunczyk opened this issue Apr 3, 2023 · 6 comments
Labels
P: default Priority: default. Default priority for new issues, to be replaced given sufficient information. T: feature request Type: feature reguest. A new feature for the project. W: done Workflow: done. This issue is done/close.

Comments

@BeataZdunczyk
Copy link
Member

Is your feature request related to a problem? Please describe.

The current state of TrenchBoot support has diverged with what was developed for QubesOS AEM for Intel hardware with TPM 1.2. This task aims to update the work and align with the TrenchBoot boot protocol being upstreamed to GRUB and Linux kernel.

Is your feature request related to a new idea or technology that
would benefit the project? Please describe.

This issue is required to ensure Qubes OS AEM supports the most recent TrenchBoot boot protocol upstreamed to GRUB and Linux kernel, which will provide improved security and functionality.

Describe the solution you'd like

Rebase the code to the most recent work implementing Secure Launch protocol and align with the TrenchBoot boot protocol being upstreamed to GRUB and Linux kernel.

Describe alternatives you've considered

N/A

Additional context

This feature request is part of Phase 3 in TrenchBoot as Anti Evil Maid project, as outlined in the documentation: https://docs.dasharo.com/projects/trenchboot-aem-v2/.

Relevant documentation you've consulted

N/A

@BeataZdunczyk BeataZdunczyk added T: feature request Type: feature reguest. A new feature for the project. P: default Priority: default. Default priority for new issues, to be replaced given sufficient information. W: todo Workflow: todo. The issue is in the initial to do state. labels Apr 3, 2023
@rossphilipson
Copy link
Contributor

@SergiiDmytruk
Copy link
Member

Overall state

Commits on grub-sl-fc-38-beta/grub-sl-fc-38-dlstub branch have broken messages. The branch is past 2.06 release. i386/txt: Add Intel TXT core implementation commit seems to be absent there. Present commits somewhat differ.

Possible path of aligning branches

  1. Re-order commits on AEM branch to separate GRUB-changes pulled from upstream and SL-related commits.

  2. (maybe) Integrate commits with fixes for new code on grub-sl-fc-38-beta into corresponding parent commits.

  3. Match commits on both branches based on what changes they include in preparation for cherry-picking.

  4. For each group of matching patches:

    1. Cherry-pick a patch/patches from grub-sl-fc-38-beta, make sure it compiles and adjust the code if it doesn’t. Use a reasonable commit message.

    2. Cherry-pick corresponding patch(es) from AEM branch and sort out differences (types, casts, comments, equivalent code changes (like different NULL checks), use of different functions). This can result in none or multiple patches each being a relatively standalone change. Requires figuring out cause of differences (style guide compliance, previous version of SL, upstream GRUB changes, etc.).

  5. Add unique changes from each branch. This will have some conflicts as well as code won't match any pre-existing version at this point.

The end result should structurally match grub-sl-fc-38-beta version with AEM changes being interspersed among them. And commit history should be clean.

@krystian-hebel
Copy link
Member

The branch is past 2.06 release.

We have to start from what https://github.com/QubesOS/qubes-grub2 uses, which is 2.06.

Present commits somewhat differ.

Maybe a result of review done on QubesOS/qubes-grub2#13? In that case changes suggested in the review have priority over grub-sl-fc-38-beta.

  1. Re-order commits on AEM branch to separate GRUB-changes pulled from upstream and SL-related commits.

If changes on grub-sl-fc-38-beta don't depend on those post-2.06 changes, we should drop them, otherwise future bump to GRUB version used by Qubes would introduce unnecessary merge conflicts.

  1. (maybe) Integrate commits with fixes for new code on grub-sl-fc-38-beta into corresponding parent commits.

That would be the best idea, remember to keep signed-off-by lines, we don't want to just steal the code 🙂

The end result should structurally match grub-sl-fc-38-beta version with AEM changes being interspersed among them. And commit history should be clean.

This is more or less what we have to do. In the end we will be uploading patches to QubesOS/qubes-grub2#13, so it doesn't make sense to have commits that fix bugs in our previous commits, we can fix the original change instead.

@BeataZdunczyk BeataZdunczyk added W: in progress Workflow: in progress. The issue is actively being worked on. and removed W: todo Workflow: todo. The issue is in the initial to do state. labels Oct 9, 2023
@SergiiDmytruk
Copy link
Member

We have to start from what https://github.com/QubesOS/qubes-grub2 uses, which is 2.06.

That's why I thought it's relevant, but it makes no difference.

Maybe a result of review done on QubesOS/qubes-grub2#13? In that case changes suggested in the review have priority over grub-sl-fc-38-beta.

Yes, that seems to be the cause of the changes. So there is nothing to move to our side and no need to rebase it.

That would be the best idea, remember to keep signed-off-by lines, we don't want to just steal the code 🙂

Turns out, those commits were already integrated.


Changes are on intel-txt-aem-slrt branch. Cherry-picking with minor adjustments was enough. Some changes looked incomplete and didn't compile or caused warnings, TPM log was always for TPM2.0 which looked like a bug. Will go through the changes again and send PR.

@krystian-hebel
Copy link
Member

I've left some comments with focus on next steps in TrenchBoot/grub#13. @rossphilipson some of them apply to your commits in case you're interested, but they are rather small issues that shouldn't change the logic.

I also created an issue for gathering changes to the specification in TrenchBoot/documentation#23. I think it would be best to not add them immediately since we will be doing more significant changes (hopefully soon) anyway.

@BeataZdunczyk
Copy link
Member Author

Closing this issue. The release was published here: TrenchBoot/qubes-antievilmaid#8, and test results are available here: #18.

@BeataZdunczyk BeataZdunczyk added W: done Workflow: done. This issue is done/close. and removed W: in progress Workflow: in progress. The issue is actively being worked on. labels Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P: default Priority: default. Default priority for new issues, to be replaced given sufficient information. T: feature request Type: feature reguest. A new feature for the project. W: done Workflow: done. This issue is done/close.
Projects
None yet
Development

No branches or pull requests

4 participants