Re: [PATCH v2] kunit: Taint kernel if any tests run

[David Gow]

On Mon, May 2, 2022 at 2:24 AM Luis Chamberlain <mcgrof@xxxxxxxxxx> wrote:
>
> On Sun, May 01, 2022 at 11:22:38AM -0700, Luis Chamberlain wrote:
> > On Sat, Apr 30, 2022 at 11:00:19AM +0800, David Gow wrote:
> > > KUnit tests are not supposed to run on production systems: they may do
> > > deliberately illegal things to trigger errors, and have security
> > > implications (assertions will often deliberately leak kernel addresses).
> > >
> > > Add a new taint type, TAINT_KUNIT to signal that a KUnit test has been
> > > run. This will be printed as 'N' (for kuNit, as K, U and T were already
> > > taken).
> > >
> > > This should discourage people from running KUnit tests on production
> > > systems, and to make it easier to tell if tests have been run
> > > accidentally (by loading the wrong configuration, etc.)
> > >
> > > Signed-off-by: David Gow <davidgow@xxxxxxxxxx>
> >
> > There is no reason to distinguish kunit from selftests if the result is
> > the same: really make the kernel try really insane stupid things which
> > may crash it or put it into a bad state.
> >
My initial thought is that KUnit is explicitly in-kernel testing,
whereas kselftest is (at least somewhat) user-space based. My personal
feeling is that "doing weird stuff from userspace" is fundamentally
different from "doing weird stuff in the kernel". That being said, in
practice many kselftest tests load modules which do strange things,
and those could be in scope for something like that. I'd still err on
the side of only having those tests (or specifically those modules)
add the taint, rather than all selftests, but could be conveniced.

The other thing of note is that KUnit tests do often leak pointer
addresses, which again is something that's a worry in the kernel, and
harmless in userspace.

> > So no, this should be renamed to "TEST_BREAK" as I think outside of
> > selftest and kunit we may grow the kernel to do stupid things outside
> > of that domain and this gives us the flexilibilty to use that in other
> > places as well.
> >
> > It begs the question if we *should* allow userspace to volunterally say
> > "hey, we are doing really insane things, brace yourself." Why ? Well
> > because selftest has tons of modules. We either then define a macro
> > that adds the taint for them and wrap the module declaration for it,
> > or we expose a syctl to let userspace volunteer to opt-in to seggest
> > we are about to try something stupid with the kernel including loading
> > some dangeerous modules which may not have macros which taint the kernel.
> > That would let selftest taint on *any* selftest. Because we can run all
> > selftests or run one selftest.
> >
> > Then, if such sysctl is exposed, maybe we should then also use this for
> > example for blktests, fstests, fio tests, etc.

Is this what TAINT_USER is for?

I think (though haven't actually tried) writing to
/proc/sys/kernel/tainted will add whatever taint flags are needed from
userspace.

That being said, I'm not _against_ making this more general, or
standardising on the existing TAINT_USER, or similar. Personally,
though, I quite like having KUnit separate (but I am, admittedly,
biased :-) ).

Cheers,
-- David