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 support for x86_32, arm, and arm64 architectures #10

Closed
wants to merge 8 commits into from

Conversation

Projects
None yet
2 participants
@tyhicks
Copy link
Contributor

tyhicks commented Jan 14, 2019

(This is a continuation of #9)

Some hardening recommendations are dependent on the processor architecture. For example, the KSPP recommendations differ between x86_32 and x86_64.

This pull request adds the ability to reason about the architecture when constructing the checklist. It also teaches the script about x86_32, arm, and arm64 specific config recommendations.

I verified that all the example configs in config_files/ show the same number of config check failures before and after these changes are applied. Of course, the ordering of the options are changed since the ordering used to construct the checklist has been changed.

Some changes since #9 include:

  • Drop kernel version detection from the pull request
  • Rename detect_arch_and_version() to detect_arch_from_config()
  • Look for CONFIG_X86_32 and CONFIG_X86_64 when detecting x86 sub architecture
  • Restrict the accepted -a <ARCHITECTURE> values to those found in SUPPORTED_ARCHS
@a13xp0p0v

This comment has been minimized.

Copy link
Owner

a13xp0p0v commented Jan 14, 2019

Hello @tyhicks , thanks a lot for the follow-up! Let me propose some improvements.

@a13xp0p0v

This comment has been minimized.

Copy link
Owner

a13xp0p0v commented Jan 14, 2019

@tyhicks , thanks for your work again!
Let me propose one more idea. What do you think about splitting KSPP recommended settings onto 4 arch-specific configs in ./config_files/?

@tyhicks

This comment has been minimized.

Copy link
Contributor Author

tyhicks commented Jan 17, 2019

Yes, I can add 4 arch-specific configs in ./config_files/.

tyhicks added some commits Jan 11, 2019

Make the script aware of target architecture
Add the ability to parse the processor architecture from the config
file. The user can override the architecture with the -a. Additionally,
if the user wants to use the -p option to print the recommendations
without specifying a kernel config file, the -a option can be used to
print the recommendations that correspond to the specificied
architecture.

Some recommendations are architecture specific so we need to warn the
user if they're checking a kernel config for an architecture that
doesn't have any architecture specific recommendations.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Update kspp-recommendations.config to look like an x86_64 config
Add a header that will make the checker script think that it is dealing
with a x86_64 config file. Additionally, update the stackprotector
related options to reflect the >= 4.18 names.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Add support for x86_32
Differentiate between 32 and 64 bit x86. This sometimes requires a
pre-parse of the kernel config in order to detect the sub-architecture
before constructing the checklist.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Add support for arm and arm64
Check for ARM and ARM64 hardening options.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Make KSPP recommendations config x86_64 specific
Rename the file so that it is clear that the recommendations are x86-64
specific.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Add a KSPP recommendations config for x86_32
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Add a KSPP recommendations config for arm
The arm section of the KSPP Recommended_Settings wiki page contains the
following lines:

 # If building an old out-of-tree Qualcomm kernel, this is similar to
 # CONFIG_STRICT_KERNEL_RWX.
 CONFIG_STRICT_MEMORY_RWX=y

Since this option only applies to an old out-of-tree Qualcomm kernel,
it is not included in the config file.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Add a KSPP recommendations config for arm64
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>

@tyhicks tyhicks force-pushed the tyhicks:additional-architectures branch from 5829534 to f790cf5 Jan 17, 2019

@tyhicks

This comment has been minimized.

Copy link
Contributor Author

tyhicks commented Jan 17, 2019

I've rebased on top of your current tree, fixed up a few things, added what I think you were asking for in the arch-specific KSPP files, and force pushed to this branch.

@a13xp0p0v

This comment has been minimized.

Copy link
Owner

a13xp0p0v commented Jan 18, 2019

Ouch.
@tyhicks , excuse me please!
I've made a code review 3 days ago, but didn't hit "submit" button, so it is "pending" :(
I've just realized that you haven't seen my review when I looked at your rebased branch.
My fault.

Show resolved Hide resolved kconfig-hardened-check.py Outdated
arch = match.group('arch')
break

if arch == 'x86':

This comment has been minimized.

Copy link
@a13xp0p0v

a13xp0p0v Jan 18, 2019

Owner

I think we can avoid parsing the comment "# Linux/(?P\w+) \S+ Kernel Configuration".

Each target config has a unique config option describing the architecture:

  • CONFIG_X86_32,
  • CONFIG_X86_64,
  • CONFIG_ARM,
  • CONFIG_ARM64.

So we even can avoid things like X86_32 = "x86_32" above and just store strings in supported_archs.

This comment has been minimized.

Copy link
@tyhicks

tyhicks Jan 18, 2019

Author Contributor

I think we can avoid parsing the comment "# Linux/(?P\w+) \S+ Kernel Configuration".

Yeah, that makes sense.

So we even can avoid things like X86_32 = "x86_32" above and just store strings in supported_archs.

I don't follow you here. We still need some way to convey which architecture we're dealing with when, for instance, constructing the checklist. We could hardcode the architecture strings there but I don't see what that gains from having variables that we can refer to.

parser.add_argument('--debug', action='store_true', help='enable internal debug mode')
args = parser.parse_args()

if not args.arch:

This comment has been minimized.

Copy link
@a13xp0p0v

a13xp0p0v Jan 18, 2019

Owner

I would change the logic here. I think we should try to detect the arch using the config file anyway.

And I don't like overriding of the detected version with -a parameter.
IMO it's better to exit with error in case of mismatch.

I think this -a parameter should be used only if:

  • the arch is not detected in the config file (we should inform about that),
  • it's the -p mode.

What do you think?

And about -p mode...
I think now we should print the architecture name at the beginning (or all architectures if -a is not specified).

This comment has been minimized.

Copy link
@tyhicks

tyhicks Jan 18, 2019

Author Contributor

With these changes, I no longer see a need for the -a flag except when also specifying the -p flag with no config file (-c).

Also, I don't feel like printing config recommendations for all architecture is useful when -a is not specified.

This comment has been minimized.

Copy link
@a13xp0p0v

a13xp0p0v Jan 21, 2019

Owner

You are right.

My first thought was to add the "arch" column with a single value.
But that will not allow to have more complex cases (for example, some option can be supported on arm/arm64 and unsupported on x86_32/x86_64).

So I will keep your approach: filtering options during construct_checklist().

@a13xp0p0v

This comment has been minimized.

Copy link
Owner

a13xp0p0v commented Jan 18, 2019

If you don't have time/desire, I can pick up your branch and polish it myself.
Thank you again!

@tyhicks

This comment has been minimized.

Copy link
Contributor Author

tyhicks commented Jan 18, 2019

If you don't have time/desire, I can pick up your branch and polish it myself.

I won't mind if you do the polishing yourself.

Thank you again!

No problem. Thanks for all the review comments.

@a13xp0p0v a13xp0p0v force-pushed the a13xp0p0v:master branch 3 times, most recently from 41b8b25 to 8510fc0 Jan 22, 2019

@a13xp0p0v

This comment has been minimized.

Copy link
Owner

a13xp0p0v commented Jan 24, 2019

Hello @tyhicks ,

I've finished with arch support based on your work.
Do you like it?
Do you have any comments or requests?
Thanks!

@tyhicks

This comment has been minimized.

Copy link
Contributor Author

tyhicks commented Jan 24, 2019

Thanks for finishing out the work. It looks very good to me. I'll make use of the changes over the next week or so and submit new pull requests if I spot anything wrong/missing. Thanks again!

@tyhicks tyhicks closed this Jan 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.