linux: make sure all config options have the same value #90065

wizeman · 2020-06-10T15:49:49Z

Motivation for this change

Currently, kernel config options whose value is yes always override
options whose value is no.

This is not always desired.

Generally speaking, if someone defines an option to have the value
no, presumably they are disabling the option for a reason, so it's
not always OK to silently enable it due to another, probably unrelated
reason.

For example, a user may want to reduce the kernel attack surface and
therefore may want to disable features that are being enabled in
common-config.nix.

In fact, common-config.nix was already silently enabling options that
were intended to be disabled in hardened/config.nix for security
reasons, such as INET_DIAG.

By eliminating the custom merge function, these config options will
now use the default module option merge functions which make sure
that all options with the highest priority have the same value.

A user that wishes to override an option defined in common-config.nix
can currently use mkForce or mkOverride to do so, e.g.:

BINFMT_MISC = mkForce (option no);

That said, this is not going to be necessary in the future, because
the plan is for kernel config options defined in nixpkgs to use a
lower priority by default, like it currently happens for other module
options.

I've also added a commit that (along with #90057, which fixes other unrelated problems) fixes the hardened kernel builds due to said conflicting definitions.

Note: this came about due to #88946 (comment) and the following comments.
I know @teto expressed his opinion that the current merge strategy is fine as long as it's documented, but I disagree for the above reasons, and would like to discuss this issue on its own.

Things done

I've tested building the following kernels:

cc @teto @vcunat

teto · 2020-06-10T16:22:39Z

without a merge feature, it would be too easy to break kernel configs and one of my goal was to make kernel configs more composable. I believe it's of higher priority to ease enabling stuff vs disabling stuff hence the default merge favors enabling feature. I would say it is mostly a matter of documentation as it should be possible to switch the behavior quite easily (change the order to have "n" first in:

          merge = mergeAnswer [ "y" "m" "n" ];

whole common-config.nix should be a mkDefault IMO.

If you are interested, I had a roadmap here #69014

wizeman · 2020-06-10T17:28:21Z

Do you have a use case where a feature is explicitly disabled somewhere, enabled somewhere else and yet you want the result to be that the feature gets enabled?

This is exactly what we don't want to happen in the use case I mentioned before (when someone wants to disable a feature for security reasons).

IMO it's better for the build to break if there are conflicting definitions inside nixpkgs so that the issue can be investigated and the appropriate result can be selected on a case-by-case basis using priorities, since in some cases we may want the result to be enabled and in other cases to be disabled.

IMO the current merge strategy of silently choosing one value when there are conflicting definitions is extremely unintuitive, has led to undesired configurations for quite a long time (in hardened kernels and also for user configs, although I understand the latter will be fixed with mkDefault) and will very likely lead to the same problem in the future.

I agree that using mkDefault in nixpkgs is better so that user configurations take priority by default.

stale · 2020-12-08T09:06:43Z

I marked this as stale due to inactivity. → More info

teto · 2020-12-08T13:02:42Z

I would like to keep the merging functions to provide different strategies. I will write some doc about kernel configuration now that it looks easier to do so. I can try to use mkDefault as discussed instead.

lheckemann · 2020-12-12T22:24:22Z

pkgs/os-specific/linux/kernel/common-config.nix

-      INET_RAW_DIAG     = whenAtLeast "4.14" module;
-      INET_DIAG_DESTROY = whenAtLeast "4.9" yes;
+      # Use a lower priority to allow these options to be overridden in hardened/config.nix
+      INET_DIAG         = mkOverride 1100 module;


I'd prefer mkDefault here.

lheckemann · 2020-12-12T22:26:31Z

@teto if multiple places with the same priority disagree on what the option should be set to, I do think it should complain and require a human to decide what should be used.

lheckemann · 2020-12-12T22:34:58Z

Alternatively, common-config could have mkDefault mapped over it, so the result is as if it were written as

{
  IKCONFIG = mkDefault yes;
  IKCONFIG_PROC = mkDefault yes;
  …
}

rather than mkDefault over the whole set as in mkDefault { IKCONFIG = yes; IKCONFIG_PROC = yes; } — that way only the individual settings will be overridden, rather than having a single setting completely disable common-config. I still prefer only using mkDefault for ones that we know need to be overridden elsewhere in nixpkgs though: using a different value than what nixpkgs usually would should be a conscious decision on the user's behalf.

NeQuissimus · 2021-02-21T15:12:55Z

pkgs/os-specific/linux/kernel/hardened/config.nix

@@ -78,6 +78,13 @@ assert (versionAtLeast version "4.9");
  PROC_KCORE         = no; # Exposes kernel text image layout
  INET_DIAG          = no; # Has been used for heap based attacks in the past

+  # INET_DIAG=n causes the following options to not exist anymore, but since they are defined in common-config.nix,
+  # make them optional
+  INET_DIAG_DESTROY = option no;


Would INET_DIAG_DESTROY = null unset this option?

error: The option value `settings.INET_DIAG_DESTROY' in `structuredExtraConfig' is not of type `submodule'.

Maybe this could be made to work by adjusting the type a bit, but I don't think there's much point investing any effort into that when we can leave it as option no.

NeQuissimus · 2021-02-21T15:16:01Z

I would suggest adding mkDefault all over common-config.nix to be honest. The hardened config should also have MODULES = n and that is going to cause all kinds of messes anyways :)

NeQuissimus · 2021-02-21T15:17:31Z

Additionally, I would add a subtest to the test latestKernel.hardened. Even if it is a simple check again /proc/config.gz and maybe make sure the ss command fails (which should rely on INET_DIAG)

lheckemann · 2021-02-24T21:07:42Z

I'd prefer using mkDefault only where something like the hardened config requires a different setting: that way, setting options to non-stock-NixOS values will make it extra clear to the user (unlike for completely unrelated options like hardware support flags which NixOS is simply missing) that they're contradicting the "preferred" nixpkgs way.

NeQuissimus · 2021-02-25T15:09:26Z

I can agree with that reasoning. Sounds good to me.
What else do we need for this?

lheckemann · 2021-02-25T17:24:49Z

Addressing review comments
Fixing merge conflicts
Merging :)

@wizeman are you willing to do this? If not I'll pick it up sometime soon probably.

NeQuissimus · 2021-02-25T17:37:45Z

Sounds good, ping me when you want another review :)

wizeman · 2021-03-05T20:24:21Z

@lheckemann Please, feel free to pick up. I don't think I will be able to finish this anytime soon.

Currently, kernel config options whose value is "yes" always override options whose value is "no". This is not always desired. Generally speaking, if someone defines an option to have the value "no", presumably they are disabling the option for a reason, so it's not always OK to silently enable it due to another, probably unrelated reason. For example, a user may want to reduce the kernel attack surface and therefore may want to disable features that are being enabled in common-config.nix. In fact, common-config.nix was already silently enabling options that were intended to be disabled in hardened/config.nix for security reasons, such as INET_DIAG. By eliminating the custom merge function, these config options will now use the default module option merge functions which make sure that all options with the highest priority have the same value. A user that wishes to override an option defined in common-config.nix can currently use mkForce or mkOverride to do so, e.g.: BINFMT_MISC = mkForce (option no); That said, this is not going to be necessary in the future, because the plan is for kernel config options defined in nixpkgs to use a lower priority by default, like it currently happens for other module options.

The parent commit forbids conflicting kernel config options. Fix the hardened kernels by allowing options in common-config.nix to be overridden by conflicting ones in hardened/config.nix. I'm explicitly avoiding using a higher priority (e.g. using mkForce) in hardened/config.nix so that the user can easily override the options in that file.

lheckemann · 2021-03-07T19:57:27Z

Done, @NeQuissimus what do you think?

NeQuissimus · 2021-03-08T14:38:46Z

@ofborg test kernel-latest kernel-lts latestKernel.login kernel-latest-ath-user-regd latestKernel.hardened

error: The option `settings.NIXOS_TEST_BOOLEAN.tristate' has conflicting definition values: - In `structuredExtraConfig': "n" - In `structuredExtraConfig': "y" since NixOS#90065 yes does not silently win over no

ofborg bot added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 101-500 labels Jun 10, 2020

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 8, 2020

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 8, 2020

teto mentioned this pull request Dec 8, 2020

linux: mkDefault over the common config #106395

Closed

10 tasks

lheckemann approved these changes Dec 12, 2020

View reviewed changes

lheckemann mentioned this pull request Dec 12, 2020

linux: allow disabling common-config #106756

Closed

3 tasks

SuperSandro2000 added the 2.status: merge conflict label Jan 18, 2021

teto added the 6.topic: kernel label Feb 18, 2021

NeQuissimus reviewed Feb 21, 2021

View reviewed changes

wizeman added 2 commits March 7, 2021 18:27

lheckemann force-pushed the u/fix-config-merge branch from d73ae97 to d81067f Compare March 7, 2021 19:57

ofborg bot removed 6.topic: kernel 2.status: merge conflict labels Mar 7, 2021

NeQuissimus mentioned this pull request Mar 8, 2021

Discussion: Hardened kernel flags #76850

Open

NeQuissimus approved these changes Mar 8, 2021

View reviewed changes

lheckemann merged commit c762b1e into NixOS:master Mar 8, 2021

Artturin mentioned this pull request Jan 17, 2023

tests.kernel-config: fix #211260

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

linux: make sure all config options have the same value #90065

linux: make sure all config options have the same value #90065

wizeman commented Jun 10, 2020

teto commented Jun 10, 2020 •

edited

wizeman commented Jun 10, 2020

stale bot commented Dec 8, 2020

teto commented Dec 8, 2020

lheckemann Dec 12, 2020

lheckemann commented Dec 12, 2020

lheckemann commented Dec 12, 2020 •

edited

NeQuissimus Feb 21, 2021

lheckemann Mar 7, 2021 •

edited

NeQuissimus commented Feb 21, 2021

NeQuissimus commented Feb 21, 2021

lheckemann commented Feb 24, 2021

NeQuissimus commented Feb 25, 2021

lheckemann commented Feb 25, 2021

NeQuissimus commented Feb 25, 2021

wizeman commented Mar 5, 2021

lheckemann commented Mar 7, 2021

NeQuissimus commented Mar 8, 2021

linux: make sure all config options have the same value #90065

linux: make sure all config options have the same value #90065

Conversation

wizeman commented Jun 10, 2020

Motivation for this change

Things done

teto commented Jun 10, 2020 • edited

wizeman commented Jun 10, 2020

stale bot commented Dec 8, 2020

teto commented Dec 8, 2020

lheckemann Dec 12, 2020

Choose a reason for hiding this comment

lheckemann commented Dec 12, 2020

lheckemann commented Dec 12, 2020 • edited

NeQuissimus Feb 21, 2021

Choose a reason for hiding this comment

lheckemann Mar 7, 2021 • edited

Choose a reason for hiding this comment

NeQuissimus commented Feb 21, 2021

NeQuissimus commented Feb 21, 2021

lheckemann commented Feb 24, 2021

NeQuissimus commented Feb 25, 2021

lheckemann commented Feb 25, 2021

NeQuissimus commented Feb 25, 2021

wizeman commented Mar 5, 2021

lheckemann commented Mar 7, 2021

NeQuissimus commented Mar 8, 2021

teto commented Jun 10, 2020 •

edited

lheckemann commented Dec 12, 2020 •

edited

lheckemann Mar 7, 2021 •

edited