On Fri, May 13, 2022 at 1:32 AM David Gow <[email protected]> wrote:
>
> Make KUnit trigger the new TAINT_TEST taint when any KUnit test is run.
> Due to KUnit tests not being intended to run on production systems, and
> potentially causing problems (or security issues like leaking kernel
> addresses), the kernel's state should not be considered safe for
> production use after KUnit tests are run.
>
> Signed-off-by: David Gow <[email protected]>
Tested-by: Daniel Latypov <[email protected]>
Looks good to me.
There's an edge case where we might have 0 suites or 0 tests and we
still taint the kernel, but I don't think we need to deal with that.
At the start of kunit_run_tests() is the cleanest place to do this.
I wasn't quite sure where this applied, but I manually applied the changes here.
Without this patch, this command exits fine:
$ ./tools/testing/kunit/kunit.py run --kernel_args=panic_on_taint=0x40000
With it, I get
[12:03:31] Kernel panic - not syncing: panic_on_taint set ...
[12:03:31] CPU: 0 PID: 1 Comm: swapper Tainted: G N
5.17.0-00001-gea9ee5e7aed8-dirty #60
I'm a bit surprised that it prints 'G' and not 'N', but this does seem
to be the right mask
$ python3 -c 'print(hex(1<<18))'
0x40000
and it only takes effect when this patch is applied.
I'll chalk that up to my ignorance of how taint works.
On Sat, May 14, 2022 at 3:09 AM Daniel Latypov <[email protected]> wrote:
>
> On Fri, May 13, 2022 at 1:32 AM David Gow <[email protected]> wrote:
> >
> > Make KUnit trigger the new TAINT_TEST taint when any KUnit test is run.
> > Due to KUnit tests not being intended to run on production systems, and
> > potentially causing problems (or security issues like leaking kernel
> > addresses), the kernel's state should not be considered safe for
> > production use after KUnit tests are run.
> >
> > Signed-off-by: David Gow <[email protected]>
>
> Tested-by: Daniel Latypov <[email protected]>
>
> Looks good to me.
>
> There's an edge case where we might have 0 suites or 0 tests and we
> still taint the kernel, but I don't think we need to deal with that.
> At the start of kunit_run_tests() is the cleanest place to do this.
Hmm... thinking about it, I think it might be worth not tainting if 0
suites run, but tainting if 0 tests run.
If we taint even if there are no suites present, that'll make things
awkward for the "build KUnit in, but not any tests" case: the kernel
would be tainted regardless. Given Android might be having the KUnit
execution stuff built-in (but using modules for tests), it's probably
worth not tainting there. (Though I think they have a separate way of
disabling KUnit as well, so it's probably not a complete
deal-breaker).
The case of having suites but no tests should still taint the kernel,
as suite_init functions could still run.
Assuming that seems sensible, I'll send out a v4 with that changed.
> I wasn't quite sure where this applied, but I manually applied the changes here.
> Without this patch, this command exits fine:
> $ ./tools/testing/kunit/kunit.py run --kernel_args=panic_on_taint=0x40000
>
> With it, I get
> [12:03:31] Kernel panic - not syncing: panic_on_taint set ...
> [12:03:31] CPU: 0 PID: 1 Comm: swapper Tainted: G N
This is showing both 'G' and 'N' ('G' being the character for GPL --
i.e. the kernel is not tainted by proprietary modules: 'P').
Jani did suggest a better way of printing these in the v1 discussion
(printing the actual names of taints present), which I might do in a
follow-up.
> 5.17.0-00001-gea9ee5e7aed8-dirty #60
>
> I'm a bit surprised that it prints 'G' and not 'N', but this does seem
> to be the right mask
> $ python3 -c 'print(hex(1<<18))'
> 0x40000
> and it only takes effect when this patch is applied.
> I'll chalk that up to my ignorance of how taint works.
-- David
On Fri, May 13, 2022 at 8:04 PM David Gow <[email protected]> wrote:
> Hmm... thinking about it, I think it might be worth not tainting if 0
> suites run, but tainting if 0 tests run.
>
> If we taint even if there are no suites present, that'll make things
> awkward for the "build KUnit in, but not any tests" case: the kernel
> would be tainted regardless. Given Android might be having the KUnit
Actually, this is what the code does right now. I was wrong.
If there are 0 suites => not tainted.
If there are 0 tests in the suites => tainted.
For kunit being built in, it first goes through this func
206 static void kunit_exec_run_tests(struct suite_set *suite_set)
207 {
208 struct kunit_suite * const * const *suites;
209
210 kunit_print_tap_header(suite_set);
211
212 for (suites = suite_set->start; suites <
suite_set->end; suites++)
213 __kunit_test_suites_init(*suites);
214 }
So for the "build KUnit in, but not any tests" case, you'll never
enter that for-loop.
Thus you'll never call __kunit_test_suites_init() => kunit_run_tests().
For module-based tests, we have the same behavior.
If there's 0 test suites, we never enter the second loop, so we never taint.
But if there's >0 suites, then we will, regardless of the # of test cases.
570 int __kunit_test_suites_init(struct kunit_suite * const * const suites)
571 {
572 unsigned int i;
573
574 for (i = 0; suites[i] != NULL; i++) {
575 kunit_init_suite(suites[i]);
576 kunit_run_tests(suites[i]);
577 }
578 return 0;
579 }
So this change should already work as intended.
> execution stuff built-in (but using modules for tests), it's probably
> worth not tainting there. (Though I think they have a separate way of
> disabling KUnit as well, so it's probably not a complete
> deal-breaker).
>
> The case of having suites but no tests should still taint the kernel,
> as suite_init functions could still run.
Yes, suite_init functions are the concern. I agree we should taint if
there are >0 suites but 0 test cases.
I don't think it's worth trying to be fancy and tainting iff there >0
test cases or a suite_init/exit function ran.
>
> Assuming that seems sensible, I'll send out a v4 with that changed.
Just to be clear: that shouldn't be necessary.
>
> > I wasn't quite sure where this applied, but I manually applied the changes here.
> > Without this patch, this command exits fine:
> > $ ./tools/testing/kunit/kunit.py run --kernel_args=panic_on_taint=0x40000
> >
> > With it, I get
> > [12:03:31] Kernel panic - not syncing: panic_on_taint set ...
> > [12:03:31] CPU: 0 PID: 1 Comm: swapper Tainted: G N
>
> This is showing both 'G' and 'N' ('G' being the character for GPL --
I just somehow missed the fact there was an 'N' at the end there.
Thanks, I thought I was going crazy. I guess I was just going blind.
Daniel
On Sat, May 14, 2022 at 3:25 PM Daniel Latypov <[email protected]> wrote:
>
> On Fri, May 13, 2022 at 8:04 PM David Gow <[email protected]> wrote:
> > Hmm... thinking about it, I think it might be worth not tainting if 0
> > suites run, but tainting if 0 tests run.
> >
> > If we taint even if there are no suites present, that'll make things
> > awkward for the "build KUnit in, but not any tests" case: the kernel
> > would be tainted regardless. Given Android might be having the KUnit
>
> Actually, this is what the code does right now. I was wrong.
> If there are 0 suites => not tainted.
> If there are 0 tests in the suites => tainted.
>
> For kunit being built in, it first goes through this func
> 206 static void kunit_exec_run_tests(struct suite_set *suite_set)
> 207 {
> 208 struct kunit_suite * const * const *suites;
> 209
> 210 kunit_print_tap_header(suite_set);
> 211
> 212 for (suites = suite_set->start; suites <
> suite_set->end; suites++)
> 213 __kunit_test_suites_init(*suites);
> 214 }
>
> So for the "build KUnit in, but not any tests" case, you'll never
> enter that for-loop.
> Thus you'll never call __kunit_test_suites_init() => kunit_run_tests().
>
> For module-based tests, we have the same behavior.
> If there's 0 test suites, we never enter the second loop, so we never taint.
> But if there's >0 suites, then we will, regardless of the # of test cases.
>
> 570 int __kunit_test_suites_init(struct kunit_suite * const * const suites)
> 571 {
> 572 unsigned int i;
> 573
> 574 for (i = 0; suites[i] != NULL; i++) {
> 575 kunit_init_suite(suites[i]);
> 576 kunit_run_tests(suites[i]);
> 577 }
> 578 return 0;
> 579 }
>
> So this change should already work as intended.
>
> > execution stuff built-in (but using modules for tests), it's probably
> > worth not tainting there. (Though I think they have a separate way of
> > disabling KUnit as well, so it's probably not a complete
> > deal-breaker).
> >
> > The case of having suites but no tests should still taint the kernel,
> > as suite_init functions could still run.
>
> Yes, suite_init functions are the concern. I agree we should taint if
> there are >0 suites but 0 test cases.
>
> I don't think it's worth trying to be fancy and tainting iff there >0
> test cases or a suite_init/exit function ran.
>
> >
> > Assuming that seems sensible, I'll send out a v4 with that changed.
>
> Just to be clear: that shouldn't be necessary.
Agreed. I think the current behavior is acceptable, and should be
unobtrusive to Android: Joe has a patch that introduces a kernel param
which disables running KUnit tests at the suite level which would
happen before this taint occurs. So the only way the taint happens is
if we actually try to execute some test cases (whether or not the
cases actually run).
> > > I wasn't quite sure where this applied, but I manually applied the changes here.
> > > Without this patch, this command exits fine:
> > > $ ./tools/testing/kunit/kunit.py run --kernel_args=panic_on_taint=0x40000
> > >
> > > With it, I get
> > > [12:03:31] Kernel panic - not syncing: panic_on_taint set ...
> > > [12:03:31] CPU: 0 PID: 1 Comm: swapper Tainted: G N
> >
> > This is showing both 'G' and 'N' ('G' being the character for GPL --
>
> I just somehow missed the fact there was an 'N' at the end there.
> Thanks, I thought I was going crazy. I guess I was just going blind.
>
>
> Daniel