Skip to content

Algin with stock Ubuntu desktop boot assets layout#109

Merged
sil2100 merged 1 commit intocanonical:classicfrom
LaiderLai:snapcraft-diff-arch
Jun 18, 2024
Merged

Algin with stock Ubuntu desktop boot assets layout#109
sil2100 merged 1 commit intocanonical:classicfrom
LaiderLai:snapcraft-diff-arch

Conversation

@LaiderLai
Copy link
Copy Markdown

The EFI boot assets layout of the classic gadget is different from the stock Ubuntu desktop version.
It's better to align them to ensure the same user experience.

And refine Makefile and snapcraft.yaml to support the snap package building correctly with amd64 and arm64 architecture.
Confirmed the default make build still working well after the refinement.

Copy link
Copy Markdown
Contributor

@xnox xnox left a comment

Choose a reason for hiding this comment

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

Needs information

have the matching changes in snapd secboot landed to support multiple bootchains, with multiple bootassets, in different directories?

Last time I tried to update boot chains in snapd it was blocked on not being able to distinguish grubx64.efi and if it comes form Boot or Ubuntu directories.

Please link PR# of changes from snapd, such that we can only land this change, once compatible snpad is in stable.

@xnox
Copy link
Copy Markdown
Contributor

xnox commented Jan 30, 2024

Algin

Is a typo, in both the commit message and this PR title.

@LaiderLai
Copy link
Copy Markdown
Author

@xnox This PR is targeted on the 'classic' branch, which is used to deploy boot assets for classic preinstallation images.
2 PRs are working for the Ubuntu Core (snapd) branches.

I guess your comment is looking for the '22' and '24' branch.
If any misunderstanding, please correct me. Tks.

@xnox
Copy link
Copy Markdown
Contributor

xnox commented Jan 31, 2024

My understanding is that this branch is used by desktop FDE models such as

https://github.com/snapcore/models/blob/master/ubuntu-classic-2310-amd64.model

Which perform TPM FDE.

Which use snapd & secboot and thus affected by the same blocking issues as the above mentioned Ubuntu Core gadgets.

Are you trying to use classic gadget, without support for TPM FDE for image creation only using ubuntu-image tooling? If yes, then this would work but then we need more branches: classic-nofde, classic-withfde, core-withfde.

@LaiderLai
Copy link
Copy Markdown
Author

Thanks! I got your point for TPM FDE case.
As I know, the BIOS from Dell/HP/Lenovo doesn't implement the same behavior for detecting whether the shim is located at boot or . Because this part is not very clear from UEFI spec...

Let me trigger a discussion for this case with the team.

@mwhudson
Copy link
Copy Markdown

mwhudson commented Feb 1, 2024

My understanding is that this branch is used by desktop FDE models such as

I think desktop FDE uses classic-23.04 / classic-23.10 (and presumably classic-24.04 soon) via https://launchpad.net/~ubuntu-core-service/+snap/pc-classic, not this branch

@LaiderLai
Copy link
Copy Markdown
Author

@xnox , after several syncing with CE-PC and HWE, we think the FDE case can be covered.

  1. For PC devices, their EFI boot assets already working as same as the stock Ubuntu desktop version.
    Their installation image must create a boot entry by efibootmgt during the installation process. The boot entry is fixed and has no impact on FDE measurement.

  2. For IoT devices, the preinstallation image doesn't create a boot entry. It means the BIOS behavior is the key point.
    If this PR is merged, the BIOS should use EFI/boot/bootx64..efi by UEFI spec default, bootx64.efi uses the fallback function to create an Ubuntu boot entry for EFI/ubuntu/shimx64.efi, then reboots to use the fixed Ubuntu boot entry to avoid the FDE measurement problem.

But, we found BIOSs from IoT projects did not follow UEFI spec. They auto-detect if EFI//shimx64.efi exists, then create and boot the Ubuntu boot entry for EFI/ubuntu/shimx64.efi. If all BIOSs auto-detect shimx64.efi and create a fixed boot entry to use, then the FDE case can be covered.

However, maybe there are some BIOSs that not only fetch EFI/ubuntu/shimx64.eif, but also skip to create the boot entry.
(Not found at this moment)
This is a case for FDE.

An idea is our FDE process should detect whether if Ubuntu boot entry exists or not.
If not, the process creates and sets it as 1st boot order then reboots. What do you think?
This is why we think the FDE case can be covered. Tks.

@LaiderLai
Copy link
Copy Markdown
Author

@mwhudson Thanks for your information. Do you know the branch maintenance plan in the future?

If the branch must be created for newer series (such as 23.04 a branch, 23.10 a branch, 24.04... and so on), the current target branch (class) is only be used for Jammy's previous series. Then it's unnecessary to consider the FDE case in this PR.

If the new series' branch will be merged back to the classic branch, we still need to consider the FDE cases.

BTW, do you know how many teams reference this repo? Core/Foundation/CE/?

Tks.

@xnox
Copy link
Copy Markdown
Contributor

xnox commented Mar 1, 2024

@LaiderLai I am no longer @ Canonical. Feel free to do whatever, but do test things extensively including refresh of gadget with TPM-FDE to exercise reseal flow.

@mwhudson
Copy link
Copy Markdown

@mwhudson Thanks for your information. Do you know the branch maintenance plan in the future?

I do not.

If the branch must be created for newer series (such as 23.04 a branch, 23.10 a branch, 24.04... and so on), the current target branch (class) is only be used for Jammy's previous series. Then it's unnecessary to consider the FDE case in this PR.

Yes, I think that's right.

If the new series' branch will be merged back to the classic branch, we still need to consider the FDE cases.

I do not know the plan here. Maybe I should...

BTW, do you know how many teams reference this repo? Core/Foundation/CE/?

I assume you mean "this branch" here? I do not know.

@kukrimate
Copy link
Copy Markdown
Member

kukrimate commented May 24, 2024

I've reviewed and tested this, and the ESP file layout itself seems fine.

But I have a few concerns:

  • Like the comment I left on the internal "The IoT x86 EFI System Partition (ESP) Layout" spec, there seems to be some confusion on around (TPM) FDE on this thread and there.

    1. Regular (non-TPM) FDE uses identical bootloader layout to non-encrypted and needs no special consideration (we don't care about encrypted /boot)
    2. TPM FDE does use a very different layout (the one in the spec is wrong), but it uses a different branch of the pc-gadget (see classic-23.10 for the current version), so it is not a concern here.
  • https://bugs.launchpad.net/ubuntu-image/+bug/2032586 is effectively a release critical bug for this, as ubuntu-image created images have a broken bootloader setup, despite the ESP layout

  • (General comment on pc-gadget, Maybe the ESP should be bigger than 50M?)

  • (Only a nitpick: Possibly squash the two commits down into one to make it easier to read this PR?)

 - BOOT
  - BOOT{ARCH}.EFI
  - fbx{ARCH}.efi
  - mm{ARCH}.efi
 - ubuntu
  - BOOT{ARCH}.CSV
  - grub.cfg
  - grub{ARCH}.efi
  - mm{ARCH}.efi
  - shim{ARCH}.efi
* Refine Makefile and snapcraft.yaml to support different archtecture build.

Signed-off-by: Laider Lai <laider.lai@canonical.com>
@LaiderLai LaiderLai force-pushed the snapcraft-diff-arch branch from 6da08c5 to 3c9f3d7 Compare June 6, 2024 05:54
@LaiderLai
Copy link
Copy Markdown
Author

Thanks for the review. Please help to check my comments inline.

I've reviewed and tested this, and the ESP file layout itself seems fine.

But I have a few concerns:

  • Like the comment I left on the internal "The IoT x86 EFI System Partition (ESP) Layout" spec, there seems to be some confusion on around (TPM) FDE on this thread and there.

    1. Regular (non-TPM) FDE uses identical bootloader layout to non-encrypted and needs no special consideration (we don't care about encrypted /boot)
    2. TPM FDE does use a very different layout (the one in the spec is wrong), but it uses a different branch of the pc-gadget (see classic-23.10 for the current version), so it is not a concern here.
      [Laider] Thanks to know the TPM FDE adopt another layout (similar to Core), which will be maintained in series branches.
      Then the TPM FDE is out of classic branch scope for this PR.
  • (General comment on pc-gadget, Maybe the ESP should be bigger than 50M?)
    [Laider] @sil2100 @kukrimate, I found the stock Noble allocated 1G size to ESP.
    I sync it into the latest commit of this PR.
    But we're not sure why the stock Noble enlarged it to 1G. It's quite big from my point of view.
    Do you know any story about this?
  • (Only a nitpick: Possibly squash the two commits down into one to make it easier to read this PR?)
    [Laider] DONE

Copy link
Copy Markdown
Contributor

@sil2100 sil2100 left a comment

Choose a reason for hiding this comment

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

I chatted with Mate and he recommended that for a preinstalled image 128-256MB ESP should be enough, 1GB might be a bit too big. Can we tweak that?

@sil2100
Copy link
Copy Markdown
Contributor

sil2100 commented Jun 18, 2024

Maybe like this: I'll merge this as-is and then change it myself.

@sil2100 sil2100 merged commit 39c0905 into canonical:classic Jun 18, 2024
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.

5 participants