Algin with stock Ubuntu desktop boot assets layout

LaiderLai commented 10 months ago

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.

xnox commented 9 months ago

Algin

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

LaiderLai commented 9 months ago

@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.

https://github.com/snapcore/pc-gadget/pull/106 for '22' branch
https://github.com/snapcore/pc-gadget/pull/107 for '24' branch

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

xnox commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 8 months ago

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

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.
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 commented 8 months ago

@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 commented 8 months ago

@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 commented 7 months ago

@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 commented 5 months ago

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?)

LaiderLai commented 5 months ago

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.

Regular (non-TPM) FDE uses identical bootloader layout to non-encrypted and needs no special consideration (we don't care about encrypted /boot)

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.

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 [Laider] Please help to check my comment from https://bugs.launchpad.net/ubuntu-image/+bug/2032586/comments/8. We have a solution currently and are looking for ubuntu-image to handle it in the future.

(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

sil2100 commented 4 months ago

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

canonical / pc-gadget

Algin with stock Ubuntu desktop boot assets layout #109