Message-ID: <CABi2SkV2_-LYGjROm3cs8nHrzBiw7pjpe0i7QGNPsPKHSeajog@mail.gmail.com>
Date: Thu, 27 Feb 2025 16:55:06 -0800
From: Jeff Xu <jeffxu@...omium.org>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, Kees Cook <keescook@...omium.org>, 
	Jann Horn <jannh@...gle.com>, Linus Torvalds <torvalds@...ux-foundation.org>, 
	Vlastimil Babka <vbabka@...e.cz>, "Liam R. Howlett" <Liam.Howlett@...cle.com>, 
	Adhemerval Zanella Netto <adhemerval.zanella@...aro.org>, Oleg Nesterov <oleg@...hat.com>, 
	Andrei Vagin <avagin@...il.com>, Benjamin Berg <benjamin@...solutions.net>, 
	LKML <linux-kernel@...r.kernel.org>, 
	"linux-hardening@...r.kernel.org" <linux-hardening@...r.kernel.org>, linux-mm@...ck.org, 
	Jorge Lucangeli Obes <jorgelo@...omium.org>, Stephen Röttger <sroettger@...gle.com>, 
	"hch@....de" <hch@....de>, "ojeda@...nel.org" <ojeda@...nel.org>, 
	Thomas Weißschuh <thomas.weissschuh@...utronix.de>, 
	"adobriyan@...il.com" <adobriyan@...il.com>, 
	"johannes@...solutions.net" <johannes@...solutions.net>, Pedro Falcato <pedro.falcato@...il.com>, 
	"hca@...ux.ibm.com" <hca@...ux.ibm.com>, Matthew Wilcox <willy@...radead.org>, 
	"anna-maria@...utronix.de" <anna-maria@...utronix.de>, "mark.rutland@....com" <mark.rutland@....com>, 
	"linus.walleij@...aro.org" <linus.walleij@...aro.org>, "Jason@...c4.com" <Jason@...c4.com>, Helge Deller <deller@....de>, 
	Randy Dunlap <rdunlap@...radead.org>, "davem@...emloft.net" <davem@...emloft.net>, 
	Peter Xu <peterx@...hat.com>, "f.fainelli@...il.com" <f.fainelli@...il.com>, 
	"gerg@...nel.org" <gerg@...nel.org>, 
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>, Ingo Molnar <mingo@...nel.org>, 
	"ardb@...nel.org" <ardb@...nel.org>, "mhocko@...e.com" <mhocko@...e.com>, 
	"42.hyeyoo@...il.com" <42.hyeyoo@...il.com>, "peterz@...radead.org" <peterz@...radead.org>, 
	"ardb@...gle.com" <ardb@...gle.com>, Elliott Hughes <enh@...gle.com>, 
	"rientjes@...gle.com" <rientjes@...gle.com>, Guenter Roeck <groeck@...omium.org>, 
	Michael Ellerman <mpe@...erman.id.au>, 
	Alexander Mikhalitsyn <aleksandr.mikhalitsyn@...onical.com>, 
	Mike Rapoport <mike.rapoport@...il.com>
Subject: Re: your mail

Hi Lorenzo,

On Tue, Feb 25, 2025, 9:43 PM Lorenzo Stoakes
>
> > > 2. Tests - could you please add some tests to assert that mremap() fails
> > >    for VDSO for instance? You've edited an existing test for VDSO in x86 to
> > >    skip the test should this be enabled, but this is not the same as a self
> > >    test. And obviously doesn't cover arm64.
> > >
> > >    This should be relatively strightforward right? You already have code
> > >    for finding out whether VDSO is msealed, so just use that to see if you
> > >    skip, then attempt mremap(), mmap() over etc. + assert it fails.
> > >
> > >    Ideally these tests would cover all the cases you've changed.
> > >
> > It is not as easy.
> >
> > The config is disabled by default. And I don't know a way to detect
> > KCONFIG  from selftest itself. Without this, I can't reasonably
> > determine the test result.
>
> Please in future let's try to get this kind of response at the point of the
> request being made :) makes life much easier.
>
There might be miscommunication ?
This version is the first time you ask about adding a self test.

IIRC, you had comments about providing the details of what tests I did, in V4.
As a follow-up, I added a test-info section in the cover letter of V5

Though I have thought about adding a selftest since the beginning,
there are two problems:
1> This config is by default disabled,
2> No good pattern to check KCONFIG from selftest.

>
> So I think it is easy actually. As I say here (perhaps you missed it?) you
> literally already have code you added to the x86 test to prevent test
> failure.
>
> So you essentially look for [vdso] or whatever, then you look up to see if
> it is sealed, ensure an mremap() fails.
>
This suggestion doesn't test the core change of this series, which is
to enable mseal for vdso.

When the vdso is marked with "sl", mremap() will fail, that's part of
the mseal() business logic and belongs in mseal_test. The mseal_test
already covers the mseal, and this series doesn't change mseal.

As for the "sl", as I responded in the "refactor mseal_test" [1] , it
will be part of the verifying step:

[1] https://lore.kernel.org/all/CABi2SkUv_1gsuGh86AON-xRLAggCvDqJMHxT17mGy-XutSTAwg@mail.gmail.com/

> Of course this doesn't check to see if it _should_ be enabled or not. I'm
> being nice by not _insisting_ we export a way for you to determine whether
> this config option is enabled or not for the sake of a test (since I don't
> want to hold up this series).
>
> But that'd be nice! Maybe in a
> /sys/kernel/security endpoint...
>
>
> ...but I think we can potentially add this later on so we don't hold things
> up here/add yet more to the series. I suspect you and Kees alike would
> prioritise getting _this series_ in at this point :)
>
> You could, if you wanted to, check to see if /proc/config.gz exists and
> zgrep it (only there if CONFIG_IKCONFIG_PROC is set) and assert based on
> that, but you know kinda hacky.

Ya, it is hacky. None of the existing selftest uses this pattern, and
I'm not sure /proc/config.gz is enabled in the default kernel config.

One option is to have ChromeOS or Android to maintain an out-of-tree
test, since the configuration will be enabled there.

Option two is to create a new path:
tools/testing/selftests/sealsysmap. Then, add
CONFIG_SEAL_SYSTEM_MAPPING=y to the config file and add a selftest to
this path. This seems to be the preferred way by selftest, but we need
a new dir for that.

Option three is to add a self-test when we have a process-level opt-in
solution. This would allow the test to deterministically know whether
the vdso should be sealed or not.

>
> But anyway, FWIW I think it'd be useful to assert that mremap() or munmap()
> fails here for system mappings for the sake of it. I guess this is, in
> effect, only checking mseal-ing works right? But it at least asserts the
> existence of the thing, and that it behaves.
>
> Later on, when you add some way of observing that it's enabled or not, you
> can extend the test to check this.

I think it is best that we don't add a test that doesn't actually
check the code change. Do you think one of the above three options
works ? maybe the second option (with a new path) ?

In any case, I think the risk is low, and the code changes are quite
simple, and fully tested.

Thanks.
-Jeff.

Powered by blists - more mailing lists