Message-ID: <202501061647.6C8F34CB1A@keescook>
Date: Mon, 6 Jan 2025 17:12:46 -0800
From: Kees Cook <kees@...nel.org>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: jeffxu@...omium.org, akpm@...ux-foundation.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>,
	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 Fri, Jan 03, 2025 at 09:38:10PM +0000, Lorenzo Stoakes wrote:
> On Tue, Dec 17, 2024 at 02:18:53PM -0800, Kees Cook wrote:
> > 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
> 
> OK let's not get ahead of ourselves :)
> 
> VDSOs/gate VMAs are treated quite differently by different arches. So we
> have to tread _very_ carefully here.
> 
> I believe PPC doe some 'tricky' things and may actually want to unmap, for
> instance.
> 
> The problem with this kind of change is we're doing something fundamental
> that impacts _every possible combinatorial combination of configs, arches,
> and use cases_ for each of these which we seeming - just assume - will have
> no issue with this.
> 
> This is insufficient, deeply. We need:
> 
> 1. Strong justification (hand waving won't suffice).
> 2. Very extensive testing and checking, and _proof_ of this testing being
> performed.
> 3. Buy-in from arch maintainers.
> 
> So far this series has provided none of those. This is why I am cautious
> and pushing back here.

Sure, I agree. This is why I was suggested the ...ARCH_HAS... Kconfig.
That will provide the way for 3) to happen. 1) just needs a little more
details in the commit log, I guess? The goal is attack surface reduction
in userspace, and remapping shenanigans have become a recent avenue of
attack.

For 2) there are limits. As you say we may have "every possible
combinatorial combination", which may not be feasible to test. But
making it available for the common cases (and of course testing those)
makes sense.

> And I absolutely will not accept a user being able to turn on a switch in a
> known-broken configuration. This is absolutely unacceptable.

Sure, of course.

> It's equally unacceptable for a user to enable a feature that is
> untested/confirmed on an architecture.

Agreed.

> So let's be careful about Linus's edict here - the operative part being 'if
> it doesn't break things'.

Right -- I should clarify: I don't mean to say "it should be enabled by
default", I meant to say that we have a common pattern for making these
kinds of features available without hiding them behind a build-time
Kconfig that would have put the features out of reach for system owners
that only use distro kernels, etc. I was pushing back on an earlier
comment that I interpreted as rejecting boot params. A boot param (when
other aspects of the system are sane) is needed for this kind of thing,
and is the pattern we use for providing optional features that distros
can make available without enabling them by default.

> > 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.
> 
> Again, I hate to push on this, but I am simply not going to allow users to
> enable features we know break things.
> 
> Users might not be aware this feature is broken for CRIU, and X, and Y and
> whatever else we've not thought about and enable it thinking it helps
> security, and end up with a broken system.

This will never be a bright line, and I think choice is more important.
For example, Ubuntu builds with CRIU, but only a tiny set of tools
actually use it. (I've actually been considering adding a boot param to
disable CRIU features since they undermine some aspects of userspace
security.)

Regardless, yes, if we can make this work with CRIU (which I thought
there seem to be consensus on), let's do it.

> This seems like putting the onus on CRIU users to deal with a known-broken
> thing? That seems really unreasonable? And people would just have to have
> the right userland code to work in the kernel with mseal?
> 
> Yeah I oppose entirely this unless I'm missing something?

Hm, well, the primary goal is for Chrome OS and Android to use this. If
there is honestly no path forward with CRIU, then hard Kconfig conflict
it is. I'd much rather have it available for anyone who wants it, just
like we do with lots of other features. Why force people who want this
and not CRIU to build their own kernels? We have all kinds of boot params
that if you set you get a broken system.

-Kees

-- 
Kees Cook

Powered by blists - more mailing lists