Add Secure Boot Kernel configuration #298

davidpil2002 · 2022-11-13T15:42:32Z

In order to support the Secure Boot feature it required some modifications when building the Linux Kernel.
This PR contained the kernel configuration aggregations to support it.

sonic-buildimage PR link: sonic-net/sonic-buildimage#12692
HLD: sonic-net/SONiC#1028

paulmenzel

Please amend the commit message, detailing the Secure Boot problem, how it was tested tested (also QEMU?), and why it can’t be enabled unconditionally.

Please add a space after ].

Makefile

patch/secure_boot_kernel_config.sh

davidpil2002

fix few typos

saiarcot895 · 2022-11-16T18:07:16Z

So Debian's/Ubuntu's approach to supporting Secure Boot is to sign the kernel after it's built with a shim binary. Could that work here, or is this approach to instead support any arbitrary public key that might be present in the UEFI?

patch/secure_boot_kernel_config.sh

saiarcot895 · 2022-11-17T03:22:02Z

Regarding the manage-config script issue, I've raised #300 to fix it.

davidpil2002 · 2022-11-17T09:29:36Z

So Debian's/Ubuntu's approach to supporting Secure Boot is to sign the kernel after it's built with a shim binary. Could that work here, or is this approach to instead support any arbitrary public key that might be present in the UEFI?

yes,
We are doing the full Secure Boot flow like the Debian approach suggested, using shim, grub, and kernel signed, also the feature is signing kernel modules.
more details can be found in the HLD(link in the description)
And yes, by using shim you can store any arbitrary pub key in the UEFI DB

davidpil2002 · 2022-11-17T09:29:53Z

Regarding the manage-config script issue, I've raised #300 to fix it.

Thanks!

davidpil2002 · 2022-11-24T16:20:10Z

Regarding the manage-config script issue, I've raised #300 to fix it.

Thanks!

…ature & Secure warmboot

…defined verification

…will modify the kconfig-inclusions/exclusions file if the Secure Boot is enabled only.

liat-grozovik · 2022-12-26T10:24:56Z

@saiarcot895 could you please add a label 'request for 202211'?

liat-grozovik · 2023-01-03T18:46:23Z

@saiarcot895 can you please help to merge and label?

lguohan

the approach here is complicated, secure boot kernel configuration should be incorprated into manage-config script, for you can add option in manage-config to enable and disable secure boot and then have a secureboot_config to have those options to be included like other existing config.

liat-grozovik · 2023-01-11T15:03:25Z

@davidpil2002 please update your branch to the latest
@saiarcot895 are you ok with the change and can approve it?

saiarcot895 · 2023-01-11T15:50:51Z

@saiarcot895 are you ok with the change and can approve it?

No. I concur with Guohan's comment.

The changes that the new script is making results in modifying files that are tracked in the repo. This means after a build (or even attempted build, in case of a build failure), the git repo's state will be dirty (because there are changes to tracked files).
There's nothing undoing these changes after the build is done, meaning if someone switches from a secure boot to non-secure boot environment variable settings, then they'll need to reset the git repo as well.
Considering all that is being done is setting some configs (one of which takes in user input), having it be done in manage-config makes more sense IMO. kconfig-secure-boot-inclusions and kconfig-secure-boot-exclusions can be created as well to store the config changes that don't depend on user input.

davidpil2002 · 2023-01-16T14:20:54Z

2. nothing undoing these changes after the build is done, meaning if someone switches from a secure boot to non-secure boot environment variable settings, then they'll need to reset the git repo as well.

I see your point.

if there is a fail, the makefile always removes the Linux repo and does wget of the kernel repo fresh. So no see a problem here, pls see the existing code line below from the Makefile:
$(addprefix $(DEST)/, $(MAIN_TARGET)): $(DEST)/% :
# Obtaining the Debian kernel source
rm -rf $(BUILD_DIR)
you are correct about the case of kernel build success because the kernel target file will contain SB (secure boot)kernel flags and will not re-build itself when the user triggers the build again with an unsecured build flags.
I'm not sure to move the logic to manage-config will fix the issue (I will try your suggestion locally anway) for the following reason:
The SB depends on the user build configuration because the user needs to decide if SB is enabled or disabled at start of build time.
So I am not sure 100% that creating kconfig-secure-boot-inclusions and kconfig-secure-boot-exclusions can solve the problem.
The reason is because the kernel configuration that the feature required is not static, it depends on the user configuration selected at the start of the build time.
Maybe until today, the sonic-Linux-kernel supported just static configuration and now we need a new solution that removes the Linux target if some user build flag related was changed?

saiarcot895 · 2023-01-17T16:30:34Z

if there is a fail, the makefile always removes the Linux repo and does wget of the kernel repo fresh. So no see a problem here

I was referring to the changes done in these lines to patch/kconfig-inclusions and patch/kconfig-exclusions.

sonic-linux-kernel/patch/secure_boot_kernel_config.sh

Lines 60 to 69 in 77e1842

    
           append_line_after_str $CONF_ARCH_BLOCK_REGEX "CONFIG_SYSTEM_TRUSTED_KEYS=\"$LOCAL_CERT_PEM\"" $KCONFIG_INCLUSIONS_FILE 
        
           append_line_after_str $CONF_ARCH_BLOCK_REGEX "CONFIG_MODULE_SIG_HASH=\"sha512\"" $KCONFIG_INCLUSIONS_FILE 
        
           append_line_after_str $CONF_ARCH_BLOCK_REGEX "CONFIG_MODULE_SIG_SHA512=y" $KCONFIG_INCLUSIONS_FILE 
        
           append_line_after_str $CONF_ARCH_BLOCK_REGEX "CONFIG_KEXEC_SIG_FORCE=y" $KCONFIG_INCLUSIONS_FILE 
        
           append_line_after_str $CONF_ARCH_BLOCK_REGEX "#Secure Boot" $KCONFIG_INCLUSIONS_FILE 
        
           append_line_after_str $CONF_ARCH_BLOCK_REGEX "CONFIG_SECURITY_LOCKDOWN_LSM" $KCONFIG_EXCLUSIONS_FILE 
        
           append_line_after_str $CONF_ARCH_BLOCK_REGEX "CONFIG_SECURITY_LOCKDOWN_LSM_EARLY" $KCONFIG_EXCLUSIONS_FILE 
        
           append_line_after_str $CONF_ARCH_BLOCK_REGEX "CONFIG_LOCK_DOWN_KERNEL_FORCE_NONE" $KCONFIG_EXCLUSIONS_FILE 
        
           append_line_after_str $CONF_ARCH_BLOCK_REGEX "CONFIG_LOCK_DOWN_IN_EFI_SECURE_BOOT" $KCONFIG_EXCLUSIONS_FILE 
        
           append_line_after_str $CONF_ARCH_BLOCK_REGEX "CONFIG_MODULE_SIG_SHA256" $KCONFIG_EXCLUSIONS_FILE

These files are part of the sonic-linux-kernel repo, rather than the Linux kernel repo.

The SB depends on the user build configuration because the user needs to decide if SB is enabled or disabled at start of build time.
So I am not sure 100% that creating kconfig-secure-boot-inclusions and kconfig-secure-boot-exclusions can solve the problem.
The reason is because the kernel configuration that the feature required is not static, it depends on the user configuration selected at the start of the build time.
Maybe until today, the sonic-Linux-kernel supported just static configuration and now we need a new solution that removes the Linux target if some user build flag related was changed?

I think there's two different things being touched on here. The purpose of creating kconfig-secure-boot-inclusions and kconfig-secure-boot-exclusions is to have just the static secure boot configs (i.e. everything except CONFIG_SYSTEM_TRUSTED_KEYS) in a regular plain-text file, which will then be read in by manage-config (similar to how it's reading in kconfig-inclusions and kconfig-exclusions) and apply the secure boot configs if some argument is passed in. manage-config would also separately set CONFIG_SYSTEM_TRUSTED_KEYS based on the argument value passed in.

For the kernel build from sonic-buildimage and controlling the configuration, whether the kernel will get rebuilt after it's been already built once depends on the DEP_FLAGS for the kernel build:

https://github.com/sonic-net/sonic-buildimage/blob/5e4a866e3311f859b9fe479aa379b38521de9725/rules/linux-kernel.dep#L6-L7

This variable would need to be updated to include the config variable that would be specified in sonic-buildimage to control whether secure boot is enabled and what certificate is used. That way, in a build with caching enabled, if someone switches between having secure boot enabled or disabled, or even different certificate paths, then the kernel will get rebuilt (I don't recall if this is fully checked or not in a build with caching disabled).

…ot-exclusions and patch/kconfig-secure-boot-inclusions files with manage-config.

patch/kconfig-secure-boot-inclusions

manage-config

… default debian key path.

davidpil2002 · 2023-01-25T13:05:20Z

Did your suggestions
@saiarcot895 pls can you approve/comment?

davidpil2002

move the Secure Boot kernel config to manage-config instead using an standalone bash script

manage-config

Makefile

manage-config

zhangyanzhao · 2023-01-25T21:36:00Z

@davidpil2002 can you please help to address the comments from @saiarcot895 ? After that, @saiarcot895 will do a re-check, then we should be good. Thanks.

…tificate

davidpil2002

Done all the fixes about supporting secure boot in config kernel.

liat-grozovik · 2023-01-29T12:01:39Z

@saiarcot895 could you please help to re-review and approve?

saiarcot895 · 2023-01-30T00:37:05Z

@davidpil2002 can you merge in the changes from the master branch?

saiarcot895 · 2023-01-30T00:37:27Z

@lguohan could you re-review?

lguohan · 2023-01-30T18:34:28Z

patch/kconfig-secure-boot-exclusions

+
+[amd64]
+CONFIG_MODULE_SIG_SHA256
+# For mellanox


why these are excluded? can we have more explanation for the justifications?

SHA256 is excluded because we are using SHA512. Its more secure.
there is more description in the HLD link attached in the description of this PR.
About the lockdown, we have a plan to support it in the future, for now for Mellanox its disabled.

I'm planning to move the SHA512 config to apply to all kernel builds in a future PR; this isn't necessarily secure-boot specific, and is nice to have in general.

I'm planning to move the SHA512 config to apply to all kernel builds in a future PR; this isn't necessarily secure-boot specific, and is nice to have in general.

Sound good, can we for now save the PR as is, and when you modify the general config you can remove it.

Because we are implicitly signing the kernel modules with sha512 in the sonic-buildimage:
https://github.com/sonic-net/sonic-buildimage/pull/12692/files#diff-de80d4961ffb88d808888c6d160af8717e70ec6c21675b0f5124b0d27db7a166
So, if the kernel configuration does not match, the image will not boot.

…el module verification

Update sonic-linux-kernel submodule pointer to include the following: * 6daddcf Add Secure Boot Kernel configuration ([sonic-net#298](sonic-net/sonic-linux-kernel#298)) Signed-off-by: dprital <[email protected]>

Update sonic-linux-kernel submodule pointer to include the following: * 6daddcf Add Secure Boot Kernel configuration ([#298](sonic-net/sonic-linux-kernel#298)) Signed-off-by: dprital <[email protected]>

* [secure boot]Add Linux Kernel configuration to support Secure Boot feature & Secure warmboot * [secure boot]Fix few typos * [secure boot]Fix Secure boot build flag condition by adding an extra defined verification * [secure boot]Remove WA after the fix in commit 5717c5d. The flow now will modify the kconfig-inclusions/exclusions file if the Secure Boot is enabled only. * [secure boot]Add secure boot kernel config by using kconfig-secure-boot-exclusions and patch/kconfig-secure-boot-inclusions files with manage-config. * [secure boot]removed comment, rename certificate with the name of the default debian key path. * [secure boot]Fix equal condition and add input file validation to certificate * [secure boot]Add signature force flag in kernel config, to force kernel module verification --------- Co-authored-by: Saikrishna Arcot <[email protected]>

* Fix setting a config with an already-existing conflicting value Fix setting a config value in kconfig-inclusions when there's already a conflicting existing value in defconfig. For example, setting CONFIG_SYSTEM_TRUSTED_KEYS would have no effect, because there would already be a setting for this specified by Debian's default config. With this, it _might_ be possible to remove the need for kconfig-force-inclusions, but that still needs to be checked. Signed-off-by: Saikrishna Arcot <[email protected]> * Support verifying the value of strings (and not just y/m/n settings) Becuase of kpatch-inclusions having quotes around the string, but the value from `scripts/config` having the quotes stripped, the comparison fails due to one side having quotes but the other side not having quotes. This effectively adds support for setting string kconfigs in kconfig-inclusion. Signed-off-by: Saikrishna Arcot <[email protected]> * Add Secure Boot Kernel configuration (#298) * [secure boot]Add Linux Kernel configuration to support Secure Boot feature & Secure warmboot * [secure boot]Fix few typos * [secure boot]Fix Secure boot build flag condition by adding an extra defined verification * [secure boot]Remove WA after the fix in commit 5717c5d. The flow now will modify the kconfig-inclusions/exclusions file if the Secure Boot is enabled only. * [secure boot]Add secure boot kernel config by using kconfig-secure-boot-exclusions and patch/kconfig-secure-boot-inclusions files with manage-config. * [secure boot]removed comment, rename certificate with the name of the default debian key path. * [secure boot]Fix equal condition and add input file validation to certificate * [secure boot]Add signature force flag in kernel config, to force kernel module verification --------- Co-authored-by: Saikrishna Arcot <[email protected]> --------- Signed-off-by: Saikrishna Arcot <[email protected]> Co-authored-by: Saikrishna Arcot <[email protected]> Co-authored-by: davidpil2002 <[email protected]>

* [secure boot]Add Linux Kernel configuration to support Secure Boot feature & Secure warmboot * [secure boot]Fix few typos * [secure boot]Fix Secure boot build flag condition by adding an extra defined verification * [secure boot]Remove WA after the fix in commit 5717c5d. The flow now will modify the kconfig-inclusions/exclusions file if the Secure Boot is enabled only. * [secure boot]Add secure boot kernel config by using kconfig-secure-boot-exclusions and patch/kconfig-secure-boot-inclusions files with manage-config. * [secure boot]removed comment, rename certificate with the name of the default debian key path. * [secure boot]Fix equal condition and add input file validation to certificate * [secure boot]Add signature force flag in kernel config, to force kernel module verification --------- Co-authored-by: Saikrishna Arcot <[email protected]>

Backport of #298 & #300 In order to support the Secure Boot feature it required some modifications when building the Linux Kernel. This PR contained the kernel configuration aggregations to support it. sonic-buildimage PR link: sonic-net/sonic-buildimage#14963 HLD: sonic-net/SONiC#1028

davidpil2002 force-pushed the secure_boot_support branch from b8758c7 to 1781f60 Compare November 13, 2022 15:56

paulmenzel suggested changes Nov 13, 2022

View reviewed changes

davidpil2002 mentioned this pull request Nov 14, 2022

Secure Boot HLD sonic-net/SONiC#1028

Merged

davidpil2002 commented Nov 14, 2022

View reviewed changes

saiarcot895 reviewed Nov 17, 2022

View reviewed changes

patch/secure_boot_kernel_config.sh Outdated Show resolved Hide resolved

davidpil2002 force-pushed the secure_boot_support branch 2 times, most recently from f5d8da5 to 46694a8 Compare November 17, 2022 20:17

davidpil2002 mentioned this pull request Nov 29, 2022

Add support of secure warm-boot sonic-net/sonic-utilities#2532

Merged

davidpil2002 added 4 commits December 12, 2022 08:31

[secure boot]Add Linux Kernel configuration to support Secure Boot fe…

9e53a97

…ature & Secure warmboot

[secure boot]Fix few typos

9724e9f

[secure boot]Fix Secure boot build flag condition by adding an extra …

10322c3

…defined verification

[secure boot]Remove WA after the fix in commit 5717c5d. The flow now …

77e1842

…will modify the kconfig-inclusions/exclusions file if the Secure Boot is enabled only.

davidpil2002 force-pushed the secure_boot_support branch from 46694a8 to 77e1842 Compare December 15, 2022 09:30

lguohan requested changes Jan 3, 2023

View reviewed changes

[secure boot]Add secure boot kernel config by using kconfig-secure-bo…

b6003e9

…ot-exclusions and patch/kconfig-secure-boot-inclusions files with manage-config.

DavidZagury suggested changes Jan 25, 2023

View reviewed changes

patch/kconfig-secure-boot-inclusions Outdated Show resolved Hide resolved

patch/kconfig-secure-boot-inclusions Outdated Show resolved Hide resolved

DavidZagury reviewed Jan 25, 2023

View reviewed changes

manage-config Outdated Show resolved Hide resolved

[secure boot]removed comment, rename certificate with the name of the…

b6b6c59

… default debian key path.

davidpil2002 commented Jan 25, 2023

View reviewed changes

saiarcot895 reviewed Jan 25, 2023

View reviewed changes

manage-config Outdated Show resolved Hide resolved

saiarcot895 reviewed Jan 25, 2023

View reviewed changes

Makefile Show resolved Hide resolved

saiarcot895 reviewed Jan 25, 2023

View reviewed changes

manage-config Show resolved Hide resolved

[secure boot]Fix equal condition and add input file validation to cer…

46beeb3

…tificate

davidpil2002 commented Jan 26, 2023

View reviewed changes

saiarcot895 approved these changes Jan 30, 2023

View reviewed changes

lguohan reviewed Jan 30, 2023

View reviewed changes

[secure boot]Add signature force flag in kernel config, to force kern…

fb8993f

…el module verification

lguohan approved these changes Feb 2, 2023

View reviewed changes

Merge branch 'master' into secure_boot_support

9eb7646

saiarcot895 merged commit 6daddcf into sonic-net:master Feb 2, 2023

dprital mentioned this pull request Feb 7, 2023

[submodule] Advance sonic-linux-kernel pointer sonic-net/sonic-buildimage#13707

Merged

8 tasks

sacnaik mentioned this pull request Mar 15, 2023

[202205] Add Secure Boot Kernel configuration #310

Merged

DavidZagury mentioned this pull request May 3, 2023

[202211] Add Secure Boot Kernel configuration backport #316

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Secure Boot Kernel configuration #298

Add Secure Boot Kernel configuration #298

davidpil2002 commented Nov 13, 2022 •

edited

Loading

paulmenzel left a comment

davidpil2002 left a comment

saiarcot895 commented Nov 16, 2022

saiarcot895 commented Nov 17, 2022

davidpil2002 commented Nov 17, 2022

davidpil2002 commented Nov 17, 2022

davidpil2002 commented Nov 24, 2022

liat-grozovik commented Dec 26, 2022

liat-grozovik commented Jan 3, 2023

lguohan left a comment

liat-grozovik commented Jan 11, 2023

saiarcot895 commented Jan 11, 2023

davidpil2002 commented Jan 16, 2023 •

edited

Loading

saiarcot895 commented Jan 17, 2023

davidpil2002 commented Jan 25, 2023

davidpil2002 left a comment

zhangyanzhao commented Jan 25, 2023

davidpil2002 left a comment

liat-grozovik commented Jan 29, 2023

saiarcot895 commented Jan 30, 2023

saiarcot895 commented Jan 30, 2023

lguohan Jan 30, 2023

davidpil2002 Jan 31, 2023 •

edited

Loading

saiarcot895 Jan 31, 2023

davidpil2002 Feb 1, 2023

Add Secure Boot Kernel configuration #298

Add Secure Boot Kernel configuration #298

Conversation

davidpil2002 commented Nov 13, 2022 • edited Loading

paulmenzel left a comment

Choose a reason for hiding this comment

davidpil2002 left a comment

Choose a reason for hiding this comment

saiarcot895 commented Nov 16, 2022

saiarcot895 commented Nov 17, 2022

davidpil2002 commented Nov 17, 2022

davidpil2002 commented Nov 17, 2022

davidpil2002 commented Nov 24, 2022

liat-grozovik commented Dec 26, 2022

liat-grozovik commented Jan 3, 2023

lguohan left a comment

Choose a reason for hiding this comment

liat-grozovik commented Jan 11, 2023

saiarcot895 commented Jan 11, 2023

davidpil2002 commented Jan 16, 2023 • edited Loading

saiarcot895 commented Jan 17, 2023

davidpil2002 commented Jan 25, 2023

davidpil2002 left a comment

Choose a reason for hiding this comment

zhangyanzhao commented Jan 25, 2023

davidpil2002 left a comment

Choose a reason for hiding this comment

liat-grozovik commented Jan 29, 2023

saiarcot895 commented Jan 30, 2023

saiarcot895 commented Jan 30, 2023

lguohan Jan 30, 2023

Choose a reason for hiding this comment

davidpil2002 Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

saiarcot895 Jan 31, 2023

Choose a reason for hiding this comment

davidpil2002 Feb 1, 2023

Choose a reason for hiding this comment

davidpil2002 commented Nov 13, 2022 •

edited

Loading

davidpil2002 commented Jan 16, 2023 •

edited

Loading

davidpil2002 Jan 31, 2023 •

edited

Loading