Re: [PATCH v4 1/1] exec: seal system mappings

["Liam R. Howlett" <Liam.Howlett@oracle.com>]

* Kees Cook <kees@kernel.org> [241217 17:19]:
> On Mon, Nov 25, 2024 at 08:20:21PM +0000, jeffxu@chromium.org wrote:
> > Seal vdso, vvar, sigpage, uprobes and vsyscall.
> > 
> > Those mappings are readonly or executable only, sealing can protect
> > them from ever changing or unmapped during the life time of the process.
> > For complete descriptions of memory sealing, please see mseal.rst [1].
> > 
> > System mappings such as vdso, vvar, and sigpage (for arm) are
> > generated by the kernel during program initialization, and are
> > sealed after creation.
> > [...]
> >  
> > +	exec.seal_system_mappings = [KNL]
> > +			Format: { no | yes }
> > +			Seal system mappings: vdso, vvar, sigpage, vsyscall,
> > +			uprobe.
> > +			- 'no':  do not seal system mappings.
> > +			- 'yes': seal system mappings.
> > +			This overrides CONFIG_SEAL_SYSTEM_MAPPINGS=(y/n)
> > +			If not specified or invalid, default is the value set by
> > +			CONFIG_SEAL_SYSTEM_MAPPINGS.
> > +			This option has no effect if CONFIG_64BIT=n
> 
> I know there is a v5 coming, but I wanted to give my thoughts to help
> shape it based on the current discussion threads.
> 
> The callers of _install_special_mapping() cover what is mentioned here.
> The vdso is very common (arm, arm64, csky, hexagon, loongarch, mips,
> parisc, powerpc, riscv, s390, sh, sparc, x86, um). For those with vdso,
> some also have vvar (arm, arm64, loongarch, mips, powerpc, riscv, s390,
> sparc, x86). After that, I see a few extra things, in addition to
> sigpage and uprobes as mentioned already in the patch:
> 
> arm sigpage
> arm64 compat vectors (what is this for arm?)
> arm64 compat sigreturn (what is this for arm?)
> nios2 kuser helpers
> uprobes
> 
> As mentioned in the patch, there is also the x86_64 vsyscall mapping which
> eludes a regular grep since it's not using _install_special_mapping() :)
> 
> So I guess the question is: can we mseal all of these universally under
> a common knob? Do the different uses mean we need finer granularity of
> knob, and do different architectures need flexibility here too? The
> patch handles the arch question with CONFIG_ARCH_HAS_SEAL_SYSTEM_MAPPINGS
> (which I think will be renamed with s/SEAL/MSEAL/ if I am following the
> threads). This seems a good solution to me. My question is
> about if sigpage, vectors, and sigreturn can also be included? (It seems
> like the answer is "yes", but I didn't see mention of the arm64 compat
> mappings.)
> 
> Linus has expressed the desire that security features be available by
> default if they don't break existing userspace and that they be compiled
> in if possible (rather than be behind a CONFIG) so that code paths are
> being exercised to gain the most exposure to finding bugs. To that end,
> it's best to have a kernel command line to control it if it isn't safe
> to have always enabled. This is how we've handled _many_ features so that
> the code is built into the kernel, but that end users (e.g. distro users)
> can enable/disable a feature without rebuilding the entire kernel.
> 
> For a "built into the kernel but default disabled unless enabled at boot
> time" example see:
> 
> config RANDOMIZE_KSTACK_OFFSET
>         bool "Support for randomizing kernel stack offset on syscall entry" if EXPERT
>         default y
>         depends on HAVE_ARCH_RANDOMIZE_KSTACK_OFFSET
> ...
> config RANDOMIZE_KSTACK_OFFSET_DEFAULT
>         bool "Default state of kernel stack offset randomization"
>         depends on RANDOMIZE_KSTACK_OFFSET
> ...
> #ifdef CONFIG_RANDOMIZE_KSTACK_OFFSET
> DEFINE_STATIC_KEY_MAYBE_RO(CONFIG_RANDOMIZE_KSTACK_OFFSET_DEFAULT,
>                            randomize_kstack_offset);
> ...
> early_param("randomize_kstack_offset", early_randomize_kstack_offset);
> 
> 
> For an example of the older "not built into the kernel but when built in
> can be turned off at boot time" that predated Linus's recommendation see:
> 
> config HARDENED_USERCOPY
>         bool "Harden memory copies between kernel and userspace"
> ...
> static DEFINE_STATIC_KEY_FALSE_RO(bypass_usercopy_checks);
> ...
> __setup("hardened_usercopy=", parse_hardened_usercopy);
> 
> (This should arguably be "default y" in the kernel these days, but
> whatever.)
> 
> So, if we want to have a CONFIG_MSEAL_SYSTEM_MAPPINGS at all, it should
> be "default y" since we have the ...ARCH_HAS... config already, and then
> add a CONFIG_MSEAL_SYSTEM_MAPPINGS_DEFAULT that is off by default (since
> we expect there may be userspace impact) and tie _that_ to the kernel
> command-line so that end users can use it, or system builders can enable
> CONFIG_MSEAL_SYSTEM_MAPPINGS_DEFAULT.

So we have at least two userspace uses that this will breaks: checkpoint
restore and now gVisor, but who knows what else?  How many config
options before we decide this can't be just on by default?

We're also veering off what mimmutable does in bsd, for no good reason.
At what point do we decide that it's not worth pushing this?

I agree that security (and everything else) should be turned on (or
default to on) for everyone, when it doesn't break things for users.  I
think this isn't one of those at this point - it's breaking things by
changing the behaviour.

And we really don't understand what it will impact fully - considering
v4 is still resulting in new things that will be broken.

Thanks,
Liam