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

Fix setting a config with an already-existing conflicting value #300

Conversation

saiarcot895
Copy link
Contributor

@saiarcot895 saiarcot895 commented Nov 17, 2022

Fix setting a config value in kconfig-inclusions when there's already a conflicting existing value in defconfig.

For example, trying to overwrite CONFIG_SYSTEM_TRUSTED_KEYS would have no effect, because there's already a value specified in the file, and the existing value takes precedence.

Also support verifying the value of strings (and not just y/m/n settings). CONFIG_SYSTEM_TRUSTED_KEYS takes a string value, and the current code doesn't completely handle strings.

Fixes #299.

Signed-off-by: Saikrishna Arcot sarcot@microsoft.com

@paulmenzel
Copy link
Contributor

Thank you for the patch. Could you please split

Also support verifying the value of strings (and not just y/m/n settings).

into a separate patch?

Also, could you amend the commit message and give an example, that didn’t work before, and works now?

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 <sarcot@microsoft.com>
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 <sarcot@microsoft.com>
@saiarcot895 saiarcot895 force-pushed the fix-config-setting-with-existing-values branch from 7161c29 to 7f8d898 Compare November 17, 2022 20:07
@saiarcot895
Copy link
Contributor Author

Commits have been split into two separate commits

@saiarcot895
Copy link
Contributor Author

@davidpil2002 could you confirm that with this fix, you can overwrite CONFIG_SYSTEM_TRUSTED_KEYS?

@davidpil2002
Copy link
Contributor

davidpil2002 commented Nov 24, 2022

@saiarcot895 its still not working so well, pls see the error below:

make[2]: warning: jobserver unavailable: using -j1.  Add '+' to parent make rule.
  GEN     Makefile
#
# configuration written to .config
#
make[2]: Leaving directory '/sonic/src/sonic-linux-kernel/linux-5.10.140/debian/build/build_amd64_none_amd64'

Checking removed kernel options...
No error

Checking added kernel options...
Option CONFIG_SECURITY_LOCKDOWN_LSM_EARLY should be set to [n] instead of [undef]
Option CONFIG_LOCK_DOWN_KERNEL_FORCE_NONE should be set to [n] instead of [undef]
Option CONFIG_LOCK_DOWN_IN_EFI_SECURE_BOOT should be set to [n] instead of [undef]

make[1]: *** [Makefile:62: /sonic/target/debs/bullseye/linux-headers-5.10.0-18-2-common_5.10.140-1_all.deb] Error 2
make[1]: Leaving directory '/sonic/src/sonic-linux-kernel'
[  FAIL LOG END  ] [ target/debs/bullseye/linux-headers-5.10.0-18-2-common_5.10.140-1_all.deb ]
make[1]: *** [Makefile:62: /sonic/target/debs/bullseye/linux-headers-5.10.0-18-2-common_5.10.140-1_all.deb] Error 2

In the kconfig-inclusion I add the follow values:

# Secure Boot
CONFIG_MODULE_SIG_KEY=""
CONFIG_SYSTEM_TRUSTED_KEYS="debian/path_to_cert/certificate.pem"
CONFIG_MODULE_SIG_HASH="sha512"
CONFIG_MODULE_SIG_SHA512=y
CONFIG_MODULE_SIG_SHA256=n
CONFIG_SECURITY_LOCKDOWN_LSM=n
CONFIG_SECURITY_LOCKDOWN_LSM_EARLY=n
CONFIG_LOCK_DOWN_KERNEL_FORCE_NONE=n
CONFIG_LOCK_DOWN_IN_EFI_SECURE_BOOT=n
CONFIG_KEXEC_SIG_FORCE=y

Note:
I think the issue, in this case, is that in general exists 2 ways to disable a kernel flag in Linux one by setting
CONFIG_KEXEC_SIG_FORCE=n and the other by setting:
# CONFIG_KEXEC_SIG_FORCE is not set
Maybe the code is not parsing those examples as should.

So for now, I will not include your fix.

@liat-grozovik
Copy link
Collaborator

@davidpil2002 kindly reminder to review and confirm so we can merge this one.

@davidpil2002
Copy link
Contributor

@davidpil2002 kindly reminder to review and confirm so we can merge this one.

Hi @liat-grozovik ,

I reviewed/tested locally the community fix, it's solved some config entries, but still had errors and not supported all the kernel configuration that required.
I wrote a comment with all the details 12 days ago.
Thanks
David

@saiarcot895
Copy link
Contributor Author

For CONFIG_SECURITY_LOCKDOWN_LSM_EARLY, CONFIG_LOCK_DOWN_KERNEL_FORCE_NONE, and CONFIG_LOCK_DOWN_IN_EFI_SECURE_BOOT, you'll want to specify these in patch/kconfig-exclusions instead. That'll unset these configs.

With them in patch/kconfig-inclusions, you're seeing the error because these configs are either set with y, or they're unset (which is implied to mean n); they're never explicitly set with n.

@davidpil2002
Copy link
Contributor

davidpil2002 commented Dec 8, 2022

For CONFIG_SECURITY_LOCKDOWN_LSM_EARLY, CONFIG_LOCK_DOWN_KERNEL_FORCE_NONE, and CONFIG_LOCK_DOWN_IN_EFI_SECURE_BOOT, you'll want to specify these in patch/kconfig-exclusions instead. That'll unset these configs.

With them in patch/kconfig-inclusions, you're seeing the error because these configs are either set with y, or they're unset (which is implied to mean n); they're never explicitly set with n.

well in general Linux kernel config support setting y or n.
But, I tested now like you said using the sonic patch/kconfig-exclusions, and it's working well.
So, for my POV the fix works and you are welcome to merge

@saiarcot895
@liat-grozovik

@saiarcot895 saiarcot895 merged commit 5717c5d into sonic-net:master Dec 9, 2022
@saiarcot895 saiarcot895 deleted the fix-config-setting-with-existing-values branch December 9, 2022 01:52
@davidpil2002
Copy link
Contributor

davidpil2002 commented Dec 11, 2022

@saiarcot895
I see that the fix its merged, but pls can you update the pointer of sonic-buildimage to sonic-linux-kernel to include your fix, so I can remove my WA from my code and use your fix

@saiarcot895
Copy link
Contributor Author

@davidpil2002 I can, but for using my fix, you don't need me to update the sonic-buildimage pointer. You should be able to rebase your private branch on top of the current master branch (or do a merge from the current master branch) and test your changes that way.

lguohan pushed a commit that referenced this pull request May 10, 2023
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
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.

Error when modifying kernel flag with kconfig-inclusions files
5 participants