Re: [PATCH v7 1/7] mseal, system mappings: kernel config and header change

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Tue, Feb 25, 2025 at 05:33:24PM -0800, Jeff Xu wrote:
> On Mon, Feb 24, 2025 at 10:05 PM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> > > +config ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
> > > +     bool
> > > +     help
> > > +       Control MSEAL_SYSTEM_MAPPINGS access based on architecture.
> > > +
> > > +       A 64-bit kernel is required for the memory sealing feature.
> > > +       No specific hardware features from the CPU are needed.
> > > +
> > > +       To enable this feature, the architecture needs to update their
> > > +       special mappings calls to include the sealing flag and confirm
> > > +       that it doesn't unmap/remap system mappings during the life
> > > +       time of the process. After the architecture enables this, a
> > > +       distribution can set CONFIG_MSEAL_SYSTEM_MAPPING to manage access
> > > +       to the feature.
> >
> > Architectures also need to be confirmed not to require any form of VDSO
> > relocation, which as discussed in previous series some arches appear to
> > need to do. I'd mention that here.
> >
> This might need clarification, the system mapping includes vdso, right
> ? Why the focus on vdso ?

My mistake, I thought scope was more limited than this when I first
looked. Please disregard the focus on VDSO here... :)

>
> The sentence  "... it doesn't unmap/remap system mappings during the
> lifetime of the process."  already cover what you want here, I think.
>

Right, I guess it just doesn't quite _emphasise_ it enough for me. Something
like the below would really help bring that out:

	The existing of this flag for an architecture implies that it does not
	require the remapping of these system mappings during process lifetime,
	so sealing these mappings is safe from a kernel perspective.

>
> > > +
> > > +       For complete descriptions of memory sealing, please see
> > > +       Documentation/userspace-api/mseal.rst
> > > +
> > >  config HAVE_PERF_EVENTS
> > >       bool
> > >       help
> > > diff --git a/security/Kconfig b/security/Kconfig
> > > index f10dbf15c294..15a86a952910 100644
> > > --- a/security/Kconfig
> > > +++ b/security/Kconfig
> > > @@ -51,6 +51,24 @@ config PROC_MEM_NO_FORCE
> > >
> > >  endchoice
> > >
> > > +config MSEAL_SYSTEM_MAPPINGS
> > > +     bool "mseal system mappings"
> > > +     depends on 64BIT
> > > +     depends on ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
> > > +     depends on !CHECKPOINT_RESTORE
> > > +     help
> > > +       Seal system mappings such as vdso, vvar, sigpage, uprobes, etc.
> >
> > Let's be specific here, 'etc.' could mean _anything_. Also you aren't
> > sealing most of this, let's just list what you are _actually_ sealing
> > here. Which is AFAIK VDSO only?
> >
> I will remove "etc" and list all the mappings.
>
> Those mappings are:
> vdso, vvar, vvar_vclock, vectors (arm compact-mode) and sigpage (arm
> compact-mode), uprobe.
>
> We seal all system mappings that x86-64 and arm64 have.

Perfect!

>
> > You can update this later as time goes on if/when you expand this.
> >
> > > +
> > > +       A 64-bit kernel is required for the memory sealing feature.
> > > +       No specific hardware features from the CPU are needed.
> > > +
> > > +       Note: CHECKPOINT_RESTORE, UML, gVisor, rr are known to relocate or
> > > +       unmap system mapping, therefore this config can't be enabled
> > > +       universally.
> >
> > Thanks for putting this here, appreciate it!
> >
> > Could we tweak this though? I'd like to make it crystal clear, so I don't
> > think 'note' sufficies and this sounds a little too vague.
> >
> > I think 'warning' is more appropriate here since you're breaking things for
> > people who might be unaware. And we need to say this -breaks- programs:
> >
> > WARNING: This feature breaks programs which rely on relocating or
> >          unmapping system mappings.
> >
> >          Known broken software at the time of writing includes
> >          CHECKPOINT_RESTORE, UML, gVisor and rr.
> >
> > I think this is critical.
> >
> Sure.

Perfect, thank you! Much appreciated.