Hi Igor,

On 8/12/21 6:43 PM, Igor Zhbanov wrote:

Hi Simon,

Thanks for thorough review. Will post the corrected version soon.

quoted

quoted

@@ -278,11 +279,11 @@ endchoice

 config LSM
      string "Ordered list of enabled LSMs"
-     default "landlock,lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
-     default "landlock,lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
-     default "landlock,lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
-     default "landlock,lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC
-     default "landlock,lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf"
+     default "nax,landlock,lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
+     default "nax,landlock,lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
+     default "nax,landlock,lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
+     default "nax,landlock,lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC
+     default "nax,landlock,lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf"

I don't know the policy with regard to new LSMs, but enabling this one by default is maybe a bit violent?
That said, considering the default mode is SECURITY_NAX_MODE_LOG, this would just log kernel messages and
not break existing systems, so this shouldn't be a problem.

I've just oriended on landlock and lockdown. They are handled in the similar
way. But, yes, by default NAX module doesn't block anything.
If you suggest not to do that, I can remove it.

If other LSMs are also enabled by default when added to the kernel, and keeping the idea that the default behavior
is warning-only (warning in a rate-limited fashion, as you rightfully did, so people running JIT engines as root
don't get their kernel log flooded with warnings), then I have no objection to that change.

quoted

quoted

+__setup("nax_mode=", setup_mode);
+
+static int __init setup_quiet(char *str)
+{
+     unsigned long val;
+
+     if (!locked && !kstrtoul(str, 0, &val))
+             quiet = val ? 1 : 0;

The order of the kernel parameters will have an impact on NAX behavior.

"nax_mode=1 nax_locked=1" and "nax_locked=1 nax_mode=1" will end up with the same behavior.
in the first case the mode is enforced, in the second case it isn't (well unless you changed
 the kernel configuration from the default) and it can't be enabled later either.

Is that desired?

Yes. The idea is that on boot you can set-up the proper options then block
further changing (especially turning the module off).
Initially it was implemented for sysctl parameters, so, you can change
something in init-scripts, then lock. Then, I've extended it to command-line
parameters.
I can ignore "locked" flag in setup_* functions to defer locking to sysctl
parsing. (Unless our command-line is glued by the bootloader from several
parts, so we want to lock it as early as possible...).

I may have badly made my point (especially considering I made a lot of typos when writing
that mail, for which I would like to apologize).
My sentence
	"nax_mode=1 nax_locked=1" and "nax_locked=1 nax_mode=1" will end up with the same behavior.
lacked the "not" word, and both options will NOT have the same behavior.
What I wanted to say was that I think both parameter should have the same behavior, and that
the ordering of the options shouldn't impact the end result, because order-dependent options
may confuse users.

For the matter of have a kernel commandline being the result of concatenations from multiple
sources, I think that if any attacker is able to alter part of the command line, they can
already write 'lsm=' to it and completely disable NAX, thus I'm not sure 'nax_locked' should
impact other setup_* functions.

I believe keeping the nax_locked parameter, but not checking for the 'locked' status in the other setup_*
functions should be enought, as sysctls writes will still be protected by the 'locked' variable.

Thanks.

Thanks,
Simon