Message-ID: <202412171248.409B10D@keescook>
Date: Tue, 17 Dec 2024 14:18:53 -0800
From: Kees Cook <kees@...nel.org>
To: jeffxu@...omium.org
Cc: akpm@...ux-foundation.org, keescook@...omium.org, jannh@...gle.com,
	torvalds@...ux-foundation.org, adhemerval.zanella@...aro.org,
	oleg@...hat.com, linux-kernel@...r.kernel.org,
	linux-hardening@...r.kernel.org, linux-mm@...ck.org,
	jorgelo@...omium.org, sroettger@...gle.com, ojeda@...nel.org,
	adobriyan@...il.com, anna-maria@...utronix.de, mark.rutland@....com,
	linus.walleij@...aro.org, Jason@...c4.com, deller@....de,
	rdunlap@...radead.org, davem@...emloft.net, hch@....de,
	peterx@...hat.com, hca@...ux.ibm.com, f.fainelli@...il.com,
	gerg@...nel.org, dave.hansen@...ux.intel.com, mingo@...nel.org,
	ardb@...nel.org, Liam.Howlett@...cle.com, mhocko@...e.com,
	42.hyeyoo@...il.com, peterz@...radead.org, ardb@...gle.com,
	enh@...gle.com, rientjes@...gle.com, groeck@...omium.org,
	mpe@...erman.id.au, Vlastimil Babka <vbabka@...e.cz>,
	Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
	Andrei Vagin <avagin@...il.com>,
	Dmitry Safonov <0x7f454c46@...il.com>,
	Mike Rapoport <mike.rapoport@...il.com>,
	Alexander Mikhalitsyn <aleksandr.mikhalitsyn@...onical.com>
Subject: Re: [PATCH v4 1/1] exec: seal system mappings

On Mon, Nov 25, 2024 at 08:20:21PM +0000, jeffxu@...omium.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.

For the command line name, if a namespace is desired, I'd agree that
naming this "mseal.special_mappings" is reasonable. It does change process
behavior, so I'm also not opposed to "process.mseal_special_mappings", and
it happens at exec, so "exec.mseal_special_mappings" is fine by me too.
I think the main question would be: will there be other things under the
proposed "mseal", "process", or "exec" namespace? I'd like to encourage
things being logically grouped since we have SO MANY already. :)

Also from discussions it sounds like there may need to be even finer-gain
control, likely via prctl, for dealing with the CRIU case. The proposal
is to provide an opt-out prctl with CAP_CHECKPOINT_RESTORE? I think this
is reasonable and lets this all work without a new CONFIG. I imagine it
would look like:

criu process (which has CAP_CHECKPOINT_RESTORE):
	- prctl(GET_MSEAL_SYSTEM_MAPPINGS)
	- if set:
		- remember we need to mseal mappings
		- prctl(SET_MSEAL_SYSTEM_MAPPINGS, 0)
		- re-exec with --mseal-system-mappings (or something)
	- perform the "fork a tree to restore" work
	- in each child, move around all the mappings
		- if we need to mseal mappings:
			- prctl(SET_MSEAL_SYSTEM_MAPPINGS, 1)
			- mseal each system mapping
		- eventually drop CAP_CHECKPOINT_RESTORE
		- become the restored process

Does that all sound right? If so I think Jeff has all the details needed
to spin a v5.

-Kees

-- 
Kees Cook

Powered by blists - more mailing lists