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

Add an add_x86_only_kconfig_checks and an add_arm_only_kconfig_checks function #128

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jvoisin
Copy link
Contributor

@jvoisin jvoisin commented Apr 30, 2024

Splitting the checks by arch family makes the code a tad more readable and self-contains, and makes it easier to inspect what checks are architecture-specific, instead of having the read the whole file.

… function

Splitting the checks by arch family makes the code a tad more readable and
self-contains, and makes it easier to inspect what checks are
architecture-specific, instead of having the read the whole file.
@jvoisin
Copy link
Contributor Author

jvoisin commented May 3, 2024

Is this something you're interested in @a13xp0p0v? Otherwise, I won't spend time resolving conflicts :)

@a13xp0p0v a13xp0p0v force-pushed the master branch 2 times, most recently from ea24300 to 78f5595 Compare June 2, 2024 12:49
@a13xp0p0v
Copy link
Owner

Hello @jvoisin,

This branch breaks the current order of the checks.

Let me describe the rationale, maybe we will create a better solution.

First, all checks in config_checklist are ordered by type:

  1. kconfig checks
  2. cmdline checks
  3. sysctl checks

In each type, the checks are ordered by reason:

  1. self_protection
  2. security_policy
  3. cut_attack_surface
  4. harden_userspace

In each reason, the checks are ordered by decision starting from the most credible:

  1. defconfig
  2. kspp
  3. grsec
  4. maintainer
  5. clipos
  6. lockdown
  7. a13xp0p0v

This ordering of the checks in kernel_hardening_checker/checks.py makes maintaining them much easier.

We also discussed this with @asarubbo in #113.

Does it sound reasonable?
Do you see how to improve the sorting of the checks?

@jvoisin
Copy link
Contributor Author

jvoisin commented Jun 9, 2024

I think it depends what we want to optimize for, code-wise: do we think it's easier to group checks by reason, or by architecture. I think the latter is more desirable, grouping by reason also makes sense.

What would be nice would be to actually group the checks, either in add_arm_only_kconfig_checks/add_x86_only_kconfig_checks/…, or in add_defconfig_kconfig_checks/add_kssp_kconfig_checks/… to make the grouping explicit and reduce the length of add_kconfig_checks. Happy to send a pull-request if you agree :)

@a13xp0p0v
Copy link
Owner

Thanks @jvoisin!

Explicit grouping by type + reason is a good idea.

We will return to this work when I update the checks according to the recent KSPP changes (b227085, thanks to Kees for the collaboration).

@a13xp0p0v
Copy link
Owner

By the way, I guess this refactoring will allow to do easy alphabetical sorting inside each group.

@jvoisin
Copy link
Contributor Author

jvoisin commented Jun 10, 2024

By the way, I guess this refactoring will allow to do easy alphabetical sorting inside each group.

That's the plan :>

@a13xp0p0v
Copy link
Owner

It's a big and important refactoring.
Let's return to this work after releasing a fresh version of kernel-hardening-checker.

@a13xp0p0v a13xp0p0v added planned_after_release This work is planned after the new release of the tool new_feature A new feature of the tool labels Jul 3, 2024
@a13xp0p0v a13xp0p0v removed the planned_after_release This work is planned after the new release of the tool label Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new_feature A new feature of the tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants