search

rhboot / shim-review

Reviews of shim
67 stars 131 forks source link

Shim 15.8 with NX support for Canonical #436

Closed kukrimate closed 1 month ago

kukrimate commented 2 months ago

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?

$ sha256sum shim*.efi
cbb8344f28251666fdf72b5441b0f8baa6acaa54ccf3ba7a22be1c322396761b  shimaa64.efi
b638835c84d03d7bcf9f7dcca84d2c3d1cac010c5a627283335882aeeae95222  shimaa64.nx.efi
e0998956d4af07192246ffe45ba80351dea4457d2b55b523f42562715fae9fa3  shimx64.efi
a52e66a6d58f923ae3621ff34e89f922c49210390f15fdf174c26ab1a34cdd1d  shimx64.nx.efi

What is the link to your previous shim review request (if any, otherwise N/A)?

https://github.com/rhboot/shim-review/issues/368

steve-mcintyre commented 2 months ago

Contacts are already well known.

richterger commented 2 months ago

review for ubuntu-shim-amd64+arm64-20240809

I 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

# sha256sum shim_15.8.orig.tar.bz2 
3acdba75265634458c6305c8c366f34f27f8fd3af566a6b479a7b1bea53bc569  shim_15.8.orig.tar.bz2

should be
a79f0a9b89f3681ab384865b1a46ab3f79d88b11b4ca59aa040ab03fffae80a9
# sha256sum /shim/shimx64.efi /shim-review/shimx64.efi 
e0998956d4af07192246ffe45ba80351dea4457d2b55b523f42562715fae9fa3  /shim/shimx64.efi
e0998956d4af07192246ffe45ba80351dea4457d2b55b523f42562715fae9fa3  /shim-review/shimx64.efi

sha256sum /shim/shimx64.nx.efi /shim-review/shimx64.nx.efi 
a52e66a6d58f923ae3621ff34e89f922c49210390f15fdf174c26ab1a34cdd1d  /shim/shimx64.nx.efi
a52e66a6d58f923ae3621ff34e89f922c49210390f15fdf174c26ab1a34cdd1d  /shim-review/shimx64.nx.efi
# objdump -x /shim/shimx64.efi   | grep -E 'SectionAlignment|DllCharacteristics'
SectionAlignment        00001000
DllCharacteristics      00000000

# objdump -x /shim/shimx64.nx.efi   | grep -E 'SectionAlignment|DllCharacteristics'
SectionAlignment        00001000
DllCharacteristics      00000100

# objcopy --only-section .sbat -O binary /shim/shimx64.efi /dev/stdout
sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
shim,4,UEFI shim,shim,1,https://github.com/rhboot/shim
shim.ubuntu,1,Ubuntu,shim,15.8-0ubuntu2,https://www.ubuntu.com/

# objcopy --only-section .sbat -O binary /shim/shimx64.nx.efi /dev/stdout
sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
shim,4,UEFI shim,shim,1,https://github.com/rhboot/shim
shim.ubuntu,1,Ubuntu,shim,15.8-0ubuntu2,https://www.ubuntu.com/
# openssl x509 -nnout -inform DER -text -in /shim/debian/canonical-uefi-ca.der 
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number:
            b9:41:24:a0:18:2c:92:67
        Signature Algorithm: sha256WithRSAEncryption
        Issuer: C = GB, ST = Isle of Man, L = Douglas, O = Canonical Ltd., CN = Canonical Ltd. Master Certificate Authority
        Validity
            Not Before: Apr 12 11:12:51 2012 GMT
            Not After : Apr 11 11:12:51 2042 GMT
        Subject: C = GB, ST = Isle of Man, L = Douglas, O = Canonical Ltd., CN = Canonical Ltd. Master Certificate Authority
        Subject Public Key Info:
            Public Key Algorithm: rsaEncryption
                Public-Key: (2048 bit)
        ...
            X509v3 Basic Constraints: critical
                CA:TRUE                
# openssl x509 -nnout -inform DER -text -in /shim/debian/debian-uefi-ca.der 
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number:
            ed:54:a1:d5:af:87:48:94:8d:9f:89:32:ee:9c:7c:34
        Signature Algorithm: sha256WithRSAEncryption
        Issuer: CN = Debian Secure Boot CA
        Validity
            Not Before: Aug 16 18:09:18 2016 GMT
            Not After : Aug  9 18:09:18 2046 GMT
        Subject: CN = Debian Secure Boot CA
        Subject Public Key Info:
            Public Key Algorithm: rsaEncryption
                Public-Key: (2048 bit)
        ...
            X509v3 Basic Constraints: critical
                CA:TRUE

Question: why does shim_15.8.orig.tar.bz2 in the container has a different sha256sum

julian-klode commented 2 months ago

@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): https://launchpad.net/~ubuntu-uefi-team/+archive/ubuntu/build/+packages

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.

SherifNagy commented 2 months ago

I am looking into this now days

joeyli commented 2 months ago

Hi,

Should we review the NX support in grub2 and kernel? or just review that the NX flag be set in shim binary?

SherifNagy commented 2 months ago

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

joeyli commented 2 months ago

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: If I am a reviewer, how can I know that the "entire boot stack" is NX-compatible ? Only checking the NX bit in grub2 and kernel ? Or we should check more detail? As you mentioned, patches and configurations?

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 Merge: 6f49693a6c85 a1b87d54f4e4 Author: Linus Torvalds torvalds@linux-foundation.org Date: Mon Aug 28 15:15:37 2023 -0700

Merge tag 'x86_boot_for_v6.6_rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip

Pull x86 boot updates from Borislav Petkov:
 "Avoid the baremetal decompressor code when booting on an EFI machine.

But I am not sure the above patchset is the only one necessary for NX-compatible in kernel.

SherifNagy commented 2 months ago

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: If I am a reviewer, how can I know that the "entire boot stack" is NX-compatible ? Only checking the NX bit in grub2 and kernel ? Or we should check more detail? As you mentioned, patches and configurations?

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

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 Merge: 6f49693a6c85 a1b87d54f4e4 Author: Linus Torvalds torvalds@linux-foundation.org Date: Mon Aug 28 15:15:37 2023 -0700

Merge tag 'x86_boot_for_v6.6_rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip

Pull x86 boot updates from Borislav Petkov:
 "Avoid the baremetal decompressor code when booting on an EFI machine.

But I am not sure the above patchset is the only one necessary for NX-compatible in kernel.

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

root@ubuntu-lts:/home/sherif# dmesg | grep NX
[    0.000000] NX (Execute Disable) protection: active

root@ubuntu-lts:/home/sherif# uname -a
Linux ubuntu-lts 6.8.0-41-generic #41-Ubuntu SMP PREEMPT_DYNAMIC Fri Aug  2 20:41:06 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux

Others please correct me if any of my answers are off

kukrimate commented 2 months ago

@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

joeyli commented 2 months ago

@kukrimate Hi, In README.md, the question 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?

Will Canonical add more information for what is the testing to ensure compatibility?

kukrimate commented 2 months ago

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?

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.

Will Canonical add more information for what is the testing to ensure compatibility?

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.

SherifNagy commented 1 month ago

Review of ubuntu-shim-amd64%2Barm64-20240809

Shim (Both NX and Non-NX)

GRUB2

Kernel

I can see that grub2 NX patches are in place https://git.launchpad.net/ubuntu/+source/grub2/commit/ they aren't upstream yet as far as I can tell and the NX flag is set on the fb, mm and grub2*.efi

@jsetje would it be possible to take a look at the NX grub patches?

Other than this, LGTM!

jsetje commented 1 month ago

I looked at the NX grub patches and there are no surprises there.

joeyli commented 1 month ago

Review of ubuntu-shim-amd64%2Barm64-20240809

[...snip]

Kernel

* Ephermal keys are used to sign modules, there are two more certs in kernel trust for out of kernel tree modules

* Lockdown patches are included and enabled

* UKI SBAT is provided and looks fine

* Kernel '6.8' has NX support and it's active
uname -a
Linux sherif 6.8.0-31-generic #31-Ubuntu SMP PREEMPT_DYNAMIC Sat Apr 20 00:40:06 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
grep  NX /var/log/dmesg
[    1.289798] kernel: NX (Execute Disable) protection: active

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.

kukrimate commented 1 month ago

We've got the signed shims back, closing.

grep  NX /var/log/dmesg
[    1.289798] kernel: NX (Execute Disable) protection: active

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.

SherifNagy commented 1 month ago

We've got the signed shims back, closing.

grep  NX /var/log/dmesg
[    1.289798] kernel: NX (Execute Disable) protection: active

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

joeyli commented 1 week ago

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.

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 https://lore.kernel.org/lkml/20230912090051.4014114-17-ardb@google.com/

All commit id in v6.7 kernel:

5f51c5d0e905608ba7be126737f7c84a793ae1aa x86/efi: Drop EFI stub .bss from .data section
7e50262229faad0c7b8c54477cd1c883f31cc4a7 x86/efi: Disregard setup header of loaded image
bfab35f552ab3dd6d017165bf9de1d1d20f198cc x86/efi: Drop alignment flags from PE section headers 768171d7ebbce005210e1cf8456f043304805c15 x86/boot: Remove the 'bugger off' message 8eace5b3555606e684739bef5bcdfcfe68235257 x86/boot: Omit compression buffer from PE/COFF image memory footprint 7448e8e5d15a3c4df649bf6d6d460f78396f7e1e x86/boot: Drop redundant code setting the root device b618d31f112bea3d2daea19190d63e567f32a4db x86/boot: Drop references to startup_64 2e765c02dcbfc2a8a4527c621a84b9502f6b9bd2 x86/boot: Grab kernel_info offset from zoffset header directly eac956345f99dda3d68f4ae6cf7b494105e54780 x86/boot: Set EFI handover offset directly in header asm 093ab258e3fb1d1d3afdfd4a69403d44ce90e360 x86/boot: Define setup size in linker script aeb92067f6ae994b541d7f9752fe54ed3d108bcc x86/boot: Derive file size from _edata symbol efa089e63b56bdc5eca754b995cb039dd7a5457e x86/boot: Construct PE/COFF .text section from assembler fa5750521e0a4efbc1af05223da9c4bbd6c21c83 x86/boot: Drop PE/COFF .reloc section 34951f3c28bdf6481d949a20413b2ce7693687b2 x86/boot: Split off PE/COFF .data section 3e3eabe26dc88692d34cf76ca0e0dd331481cc15 x86/boot: Increase section and file alignment to 4k/512

I put here for reference.