-
Notifications
You must be signed in to change notification settings - Fork 131
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
Shim 15.8 with NX support for Canonical #436
Comments
Contacts are already well known. |
review for ubuntu-shim-amd64+arm64-20240809I am not an authorized reviewer, hope this helps anyway I only have taken a look at the x86 build, because I have no arm system
Question: why does shim_15.8.orig.tar.bz2 in the container has a different sha256sum |
@richterger The reason for the tarball difference inside the Docker reproducer is that the tarball is generated from git where the original tarball was imported to, rather than the original tarball being downloaded, as it would be on the real builder. The real build is here (15.8-0ubuntu2): The Debian CA is shipped in the packaging yes, but it's not built into the shim, it's just an artifact of Ubuntu packages deriving from Debian ones. |
I am looking into this now days |
Hi, Should we review the NX support in grub2 and kernel? or just review that the NX flag be set in shim binary? |
In theory, everything , patches , configurations and so on |
hm... Yes, but I did not find any check list in shim-review project for reviewing the NX support in grub2 and kernel. In README.md, only one question is about NX bit: Do you have the NX bit set in your shim? If so, is your entire boot stack NX-compatible and what testing have you done to ensure such compatibility?The question is: And, what's the definition of NX-compatible? Does it mean just for booting success? Or we should make sure that all data memory regions be set to NX (which means only code regions be set non-NX)? I am not grub2 expert. But for kernel, I only known that a patchset in v6.6 kernel is necessary: commit bd9e99f790f21374b831a7dcf638156beacf3bf4
But I am not sure the above patchset is the only one necessary for NX-compatible in kernel. |
Well, we do need to work a bit on the README to clarify some things, also this is the first NX full chain submission, me personally, still wrapping my head around things
As far as I recall / know, there are a set of patches that introduced around 6.7x kernel, then those got added either to 6.7 or 6.8 and enabled by default, also there are some back porting to older kernels / lts kernel, the commit you mentioned, might be one of those backported sets or might be the one I am talking about, I need to dig a bit in get the correct lore link, what I recall ontop of my head they were in v15 or v14 of those patch set For Grub, I know that fedora has their own set of patches, and ubuntu now having their own as well, and there are efforts to push those upstream based on what I read in grub-devel mailing list, but not sure how far they are. I would say, the minimal is to check the that the bits are set correctly, making sure they have the patches in place " if it's upstream or ported " this will require some question, digging into change log and so on. I booted latest ubuntu LTS 24.04 image and I can see that the kernel does have NX enabled , but life got in the way and didn't check the rest of the boot chain
Others please correct me if any of my answers are off |
@SherifNagy NX in GRUB is only enabled in Oracular as of right now and not Noble, but the NX shim will of course only be deployed in Oracular up until Noble gets an NX GRUB |
@kukrimate Do you have the NX bit set in your shim? If so, is your entire boot stack NX-compatible and what testing have you done to ensure such compatibility? Will Canonical add more information for what is the testing to ensure compatibility? |
As mentioned there are two shims, one NX one not. The NX stack was tested with our NX grub and kernel using Microsoft's Mu firmware.
I would like to not edit this submission any longer. We've developed the NX kernel loader in GRUB ourselves and done the usual testing. |
Review of ubuntu-shim-amd64%2Barm64-20240809
Shim (Both NX and Non-NX)
GRUB2
Kernel
Notes:I can see that grub2 NX patches are in place @jsetje would it be possible to take a look at the NX grub patches? Other than this, LGTM! |
I looked at the NX grub patches and there are no surprises there. |
[...snip]
NX in kenrel mm subsystem be supported since long time ago. About the NX support in the mm subsystem in kernel. The nx support be introduced to 32-bit kernel since log time ago, it's before v2.6.12-rc2. And 64-bit nx support log in mm be introduced by 54e63f3a4282 patch in v2.6.30-rc1. Which means that the above kernel log shows in dmesg since log time ago. In old kernel, the noexec kernel parameter be used to enable nx in mm (noexec=on/off). After 76ea0025a214 patch in v5.19-rc1, the noexec kernel parameter be removed. So the NX support in mm can not be turned-off since v5.19 kernel. Except the kernel patchset "Avoid the baremetal decompressor code when booting on an EFI machine." in v6.6 kernel, I did not find necessary patches for NX in v6.7 and v6.8 kernel. The above information is for note. If anyone know why we need v6.8 kernel for supporting NX in kernel. Please add comments. |
We've got the signed shims back, closing.
Also a note that the above message about NX protection has nothing to do with boot time UEFI NX support. It instead refers to runtime NX support in the kernel page tables, thus is not relevant proof of UEFI NX"support. Our kernel supports UEFI NX anyway, so this isn't an issue, but I think it is worth noting for future reviews. |
Oh I see, thanks! good to know,however I did check the source and the latest NX patches are in there. Thanks for the clarification :) |
I want to correct my above statement after viewed kernel x86 patches in v6.7. I believe that Ard Biesheuvel's patches is necessary: [PATCH v2 00/15] x86/boot: Rework PE header generation All commit id in v6.7 kernel: 5f51c5d0e905608ba7be126737f7c84a793ae1aa x86/efi: Drop EFI stub .bss from .data section I put here for reference. |
Confirm the following are included in your repo, checking each box:
What is the link to your tag in a repo cloned from rhboot/shim-review?
https://github.com/canonical/shim-review/tree/ubuntu-shim-amd64%2Barm64-20240809
What is the SHA256 hash of your final SHIM binary?
What is the link to your previous shim review request (if any, otherwise N/A)?
#368
The text was updated successfully, but these errors were encountered: