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

EC keys support #1194

Merged
merged 1 commit into from
Jul 29, 2022
Merged

EC keys support #1194

merged 1 commit into from
Jul 29, 2022

Conversation

Unb0rn
Copy link
Contributor

@Unb0rn Unb0rn commented Jul 22, 2022

EC signatures require the corresponding digest length. Enabling gpg to auto select the algo and supporting those algos in kernel should fix #1066

@tlaurion
Copy link
Collaborator

tlaurion commented Jul 22, 2022

@Unb0rn added kernel config changes to other linux configs in tree master...tlaurion:heads:Unb0rn_ecc_fix_other_kernels
Not sure about qemu q35 (could not find quickly, but I think it does).

Building under CI: https://app.circleci.com/pipelines/github/tlaurion/heads/1104/workflows/8658584e-7d35-41d7-afc3-bc070c0e3e6e/jobs/8150

  • Librem 14 tested (I assume)
  • try rom on x230 (regression test for factory reset) and report back
  • Have @Unb0rn merge the changes of my branch to his commit (force push)
  • Merge

@Unb0rn
Copy link
Contributor Author

Unb0rn commented Jul 22, 2022

@tlaurion yeah, the only question-is it a good idea to add ssse3 versions of sha2 or just regular? Is the speed improvement worth the size (and does it make a big difference?)

@tlaurion
Copy link
Collaborator

tlaurion commented Jul 22, 2022

@tlaurion yeah, the only question-is it a good idea to add ssse3 versions of sha2 or just regular? Is the speed improvement worth the size (and does it make a big difference?)

@Unb0rn no clue unless tested. :)

The size increase is a desired comparison point in PR from people watching PR for references. It would be nice to add here. Kernel and initrd are being xzipped.

But we can do a quick compare between free space available in the output of coreboot build.

Space consideration is important for legacy boards, which is what I tend to check for. We are really tight on space there, and I have no clue yet on the cost of the addition, CI build will tell.

As for performance, on Heads use case, gpg is normally not used for things requiring big check-summing operations. Would be nice to do some quick benchmarks here, but as you point out, adding kernel sha512 support is the need thing here for ECC, where removing the binding to only sha256 in kexec-signing was the required thing (unless it causes regression).

@tlaurion
Copy link
Collaborator

tlaurion commented Jul 22, 2022

From last master's commit Board build output for x230-hotp-maximized board:

Name                           Offset     Type           Size   Comp
cbfs master header             0x0        cbfs header        32 none
fallback/romstage              0x80       stage           85100 none
cpu_microcode_blob.bin         0x14d80    microcode       26624 none
fallback/ramstage              0x1b600    stage           97676 none
config                         0x33400    raw               786 none
revision                       0x33780    raw               691 none
fallback/dsdt.aml              0x33a80    raw             14615 none
vbt.bin                        0x37400    raw              1433 LZMA (4281 decompressed)
cmos_layout.bin                0x37a00    cmos_layout      1884 none
fallback/postcar               0x381c0    stage           25816 none
fallback/payload               0x3e700    simple elf    7309255 none
(empty)                        0x736f00   null          4361880 none
bootblock                      0xb5fdc0   bootblock       65536 none

From my PR including linux-x230-maximized.config changes for comaprison CI board build's log:

Name                           Offset     Type           Size   Comp
cbfs master header             0x0        cbfs header        32 none
fallback/romstage              0x80       stage           85100 none
cpu_microcode_blob.bin         0x14d80    microcode       26624 none
fallback/ramstage              0x1b600    stage           97681 none
config                         0x33400    raw               786 none
revision                       0x33780    raw               691 none
fallback/dsdt.aml              0x33a80    raw             14615 none
vbt.bin                        0x37400    raw              1433 LZMA (4281 decompressed)
cmos_layout.bin                0x37a00    cmos_layout      1884 none
fallback/postcar               0x381c0    stage           25816 none
fallback/payload               0x3e700    simple elf    7317959 none
(empty)                        0x739100   null          4353176 none
bootblock                      0xb5fdc0   bootblock       65536 none

We lost (empty) 0x736f00 null 4361880 none - (empty) 0x739100 null 4353176 none

4361880 - 4353176 = 8704 bytes, not really a concern, I think (that is bzImage related changes, compressed with xz).

The changes encompasses both sha256sum and sha512sum being sse3 and while not removing non sse3 versions. This explains the size increase.

@tlaurion
Copy link
Collaborator

tlaurion commented Jul 22, 2022

@Unb0rn Do you witness any improvement in your tests with sse3 support ?

@Unb0rn
Copy link
Contributor Author

Unb0rn commented Jul 22, 2022

@tlaurion yeah, the only question-is it a good idea to add ssse3 versions of sha2 or just regular? Is the speed improvement worth the size (and does it make a big difference?)

@Unb0rn no clue unless tested. :)

The size increase is a desired comparison point in PR from people watching PR for references. It would be nice to add here. Kernel is being bzipped, where initrd is neing xzipped. The later is better.

But we can do a quick compare between free space available in the output of coreboot build.

Space consideration is important for legacy boards, which is what I tend to check for. We are really tight on space there, and I have no clue yet on the cost of the addition, CI build will tell.

As for performance, on Heads use case, gpg is normally not used for things requiring big check-summing operations. Would be nice to do some quick benchmarks here, but as you point out, adding kernel sha512 support is the need thing here for ECC, where removing the binding to only sha256 in kexec-signing was the required thing (unless it causes regression).

Yeah I got this when I tried to include btrfs support in kernel =) Size is everything now. BTW, doesn't CONFIG_KERNEL_XZ state that the bzImage is actually xz-compressed? It also looks like in Kconfig that selecting CRYPTO_SHA256_SSSE3 auto selects CRYPTO_SHA256 and of course it works for SHA512 too.

I haven't tried benchmarking ssse3 vs standard sha2, just selected HW-accelerated ones =)

@tlaurion
Copy link
Collaborator

tlaurion commented Jul 22, 2022

@Unb0rn

It also looks like in Kconfig that selecting CRYPTO_SHA256_SSSE3 auto selects CRYPTO_SHA256 and of course it works for SHA512 too.

As stated under linuxboot/heads-wiki#89 the proper way to get real config changes is to see what a change in config results when loading that config (make menuconfig) and saving that config. Then doing a make savedefconfig and moving that config over origin will tell what are the changes related to defconfig (which is what is currently saved under Heads tree (will change in the future, this is too complicated.

To sum it up, for both 4.14 and 5.10, the real .config (as what will be compiled when compiling kernel)

user@heads-tests:~/heads/build/linux-5.10.5-hardlinked$ grep CRYPTO_SHA .config
CONFIG_CRYPTO_SHA1=y
CONFIG_CRYPTO_SHA1_SSSE3=y
CONFIG_CRYPTO_SHA256_SSSE3=y
CONFIG_CRYPTO_SHA512_SSSE3=y
CONFIG_CRYPTO_SHA256=y
CONFIG_CRYPTO_SHA512=y
# CONFIG_CRYPTO_SHA3 is not set

But when doing make savedefconfig

user@heads-tests:~/heads$ grep CRYPTO_SHA config/linux-librem_common.config 
CONFIG_CRYPTO_SHA1_SSSE3=y
CONFIG_CRYPTO_SHA256_SSSE3=y
CONFIG_CRYPTO_SHA512_SSSE3=y

@tlaurion
Copy link
Collaborator

tlaurion commented Jul 22, 2022

BTW, doesn't CONFIG_KERNEL_XZ state that the bzImage is actually xz-compressed?

XZ it is.

And yet again we include in kernel stuff that are unrelated to the actual architecture (x86)....

user@heads-tests:~/heads/build/linux-5.10.5-hardlinked$ grep -i xz .config
CONFIG_HAVE_KERNEL_XZ=y
CONFIG_KERNEL_XZ=y
CONFIG_RD_XZ=y
CONFIG_INITRAMFS_COMPRESSION_XZ=y
CONFIG_XZ_DEC=y
CONFIG_XZ_DEC_X86=y
CONFIG_XZ_DEC_POWERPC=y
CONFIG_XZ_DEC_IA64=y
CONFIG_XZ_DEC_ARM=y
CONFIG_XZ_DEC_ARMTHUMB=y
CONFIG_XZ_DEC_SPARC=y
CONFIG_XZ_DEC_BCJ=y
CONFIG_XZ_DEC_TEST=m
CONFIG_DECOMPRESS_XZ=y

But all of this is for another issue and is unrelated here.

Also, the wrapper involved in XZ compression can be found under scripts/xz_wrap.sh, which is default (-6)
This page shows important differences (5mb) between default xz -6 and xz -9 for kernel compression, with little impact for powerful machines (read non-embedded CPU) at decompression. Edit: gain is neglictible, just like adding sse3 inkernel modules here (
7309255-7308231=1024 bytes gain in freed space)

Might be interesting for #590, still.

EC signatures requires that the digest has the corresponding length. Removing the hardcoded sha2-256 hash function and adding support of sha2-384 and sha2-512 should allow using EC crypto.
@tlaurion
Copy link
Collaborator

@JonathonHall-Purism I guess you should approve prior of me merging

@tlaurion
Copy link
Collaborator

@JonathonHall-Purism anything against this?

@JonathonHall-Purism
Copy link
Collaborator

Looks good to me 💯 I tested this on the Librem 14 - double checked that OEM reset still uses the same default algorithms, manually generated an ECC key and verified functionality, switched between the current PureBoot release and this PR build to verify compatibility (with RSA keys of course). No problems, merge away 🚢

@tlaurion
Copy link
Collaborator

tlaurion commented Jul 26, 2022

Might break builds for x220 non maximized (using linux-x230-legacy.conf) which is not built by CI.

Will build and report back.
Builds are the same, basically. x220 board is flashing only bios region per board instruction. Coreboot config there takes for granted that ME has been cleaned. I will merge, and if needs are brought in terms of space, that will push work for 03 and 02 being passed to Os and kernel size reduction.

@tlaurion tlaurion merged commit 21505aa into linuxboot:master Jul 29, 2022
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.

Trouble with ECC keys and GPG digest algo
3 participants