| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250611093942-f6c65a06-c72a-4451-aa1e-8cb8de0d69cb@linutronix.de>
Date: Wed, 11 Jun 2025 10:04:35 +0200
From: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
To: Nicolin Chen <nicolinc@...dia.com>
Cc: Jason Gunthorpe <jgg@...dia.com>, Shuah Khan <shuah@...nel.org>,
Shuah Khan <skhan@...uxfoundation.org>, Willy Tarreau <w@....eu>,
Thomas Weißschuh <linux@...ssschuh.net>, Kees Cook <kees@...nel.org>,
Andy Lutomirski <luto@...capital.net>, Will Drewry <wad@...omium.org>, Mark Brown <broonie@...nel.org>,
Muhammad Usama Anjum <usama.anjum@...labora.com>, linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v4 09/14] selftests: harness: Move teardown conditional
into test metadata
On Wed, Jun 11, 2025 at 12:05:25AM -0700, Nicolin Chen wrote:
> On Tue, Jun 10, 2025 at 08:46:57PM -0300, Jason Gunthorpe wrote:
> > On Tue, Jun 10, 2025 at 11:48:44AM -0700, Nicolin Chen wrote:
> > > On Tue, Jun 10, 2025 at 09:09:02AM -0300, Jason Gunthorpe wrote:
> > > > On Tue, Jun 10, 2025 at 01:38:22PM +0200, Thomas Weißschuh wrote:
> > > > > > ------------------------------------------------------------------
> > > > > > # RUN iommufd_dirty_tracking.domain_dirty128M_huge.enforce_dirty ...
> > > > > > # enforce_dirty: Test terminated unexpectedly by signal 11
> > > >
> > > > Sig 11 is weird..
> > >
> > > > > On another note, the selftest should use the kselftest_harness' ASSERT_*()
> > > > > macros instead of plain assert().
> > > >
> > > > IIRC the kselftest stuff explodes if you try to use it's assert
> > > > functions within a fixture setup/teardown context.
> > > >
> > > > I also wasn't able to reproduce this (x86 ubuntu 24 LTS OS) Maybe
> > > > it is ARM specific, I think Nicolin is running on ARM..
> > >
> > > Yes. And I was running with 64KB page size. I just quickly retried
> > > with 4KB page size (matching x86), and all failed tests pass now.
> >
> > That's a weird thing to be sensitive too. Can you get a backtrace from
> > the crash, what function/line is crashing?
>
> I think I am getting what's going on. Here the harness code has a
> parent process and a child process:
>
> --------------------------------------------------------------
> 428- /* _metadata and potentially self are shared with all forks. */ \
> 429: child = fork(); \
> 430: if (child == 0) { \
> 431- fixture_name##_setup(_metadata, self, variant->data); \
> 432- /* Let setup failure terminate early. */ \
> 433- if (_metadata->exit_code) \
> 434- _exit(0); \
> 435- *_metadata->no_teardown = false; \
> 436- fixture_name##_##test_name(_metadata, self, variant->data); \
> 437- _metadata->teardown_fn(false, _metadata, self, variant->data); \
> 438- _exit(0); \
> 439: } else if (child < 0 || child != waitpid(child, &status, 0)) { \
> 440- ksft_print_msg("ERROR SPAWNING TEST GRANDCHILD\n"); \
> 441- _metadata->exit_code = KSFT_FAIL; \
> 442- } \
> 443- _metadata->teardown_fn(true, _metadata, self, variant->data); \
> 444- munmap(_metadata->no_teardown, sizeof(*_metadata->no_teardown)); \
> 445- _metadata->no_teardown = NULL; \
> 446- if (self && fixture_name##_teardown_parent) \
> 447- munmap(self, sizeof(*self)); \
> 448- if (WIFEXITED(status)) { \
> 449- if (WEXITSTATUS(status)) \
> 450- _metadata->exit_code = WEXITSTATUS(status); \
> 451- } else if (WIFSIGNALED(status)) { \
> 452- /* Forward signal to __wait_for_test(). */ \
> 453- kill(getpid(), WTERMSIG(status)); \
> 454- } \
> ....
> 456- static void wrapper_##fixture_name##_##test_name##_teardown( \
> 457- bool in_parent, struct __test_metadata *_metadata, \
> 458- void *self, const void *variant) \
> 459- { \
> 460- if (fixture_name##_teardown_parent == in_parent && \
> 461- !__atomic_test_and_set(_metadata->no_teardown, __ATOMIC_RELAXED)) \
> 462- fixture_name##_teardown(_metadata, self, variant); \
> 463- } \
> --------------------------------------------------------------
>
> I found there is a race between those two processes, resulting in
> the teardown() not getting invoked: I added some ksft_print_msg()
> calls in-between the lines to debug, those tests can pass mostly,
> as teardown() got invoked.
>
> I think the reason why those huge page cases fail is just because
> the huge version of setup() takes longer time..
Can you try to recreate this issue with changes to
tools/testing/selftests/kselftest_harness/harness-selftest.c ?
> I haven't figured out a proper fix yet, but something smells bad:
> 1) *no_teardown is set non-atomically, while both processes calls
> __atomic_test_and_set()
Does this make a difference?
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 2925e47db995..89fb37a21d9d 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -410,7 +410,7 @@
/* Makes sure there is only one teardown, even when child forks again. */ \
_metadata->no_teardown = mmap(NULL, sizeof(*_metadata->no_teardown), \
PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0); \
- *_metadata->no_teardown = true; \
+ __atomic_store_n(_metadata->no_teardown, true, __ATOMIC_SEQ_CST); \
if (sizeof(*self) > 0) { \
if (fixture_name##_teardown_parent) { \
self = mmap(NULL, sizeof(*self), PROT_READ | PROT_WRITE, \
@@ -429,7 +429,7 @@
/* Let setup failure terminate early. */ \
if (_metadata->exit_code) \
_exit(0); \
- *_metadata->no_teardown = false; \
+ __atomic_store_n(_metadata->no_teardown, false, __ATOMIC_SEQ_CST); \
fixture_name##_##test_name(_metadata, self, variant->data); \
_metadata->teardown_fn(false, _metadata, self, variant->data); \
_exit(0); \
@@ -455,7 +455,7 @@
void *self, const void *variant) \
{ \
if (fixture_name##_teardown_parent == in_parent && \
- !__atomic_test_and_set(_metadata->no_teardown, __ATOMIC_RELAXED)) \
+ !__atomic_test_and_set(_metadata->no_teardown, __ATOMIC_SEQ_CST)) \
fixture_name##_teardown(_metadata, self, variant); \
} \
static struct __test_metadata *_##fixture_name##_##test_name##_object; \
> 2) parent doesn't seem to wait for the setup() to complete..
setup() is called in the child (L431) right before the testcase itself is
called (L436). The parent waits for the child to exit (L439) before unmapping.
> 3) when parent runs faster than the child that is still running
> setup(), the parent unmaps the no_teardown and set it to NULL,
> then UAF in the child, i.e. signal 11?
That should never happen as the waitpid() will block until the child running
setup() and the testcase itself have exited.
Does the issue also happen when you only execute a single testcase?
$ ./iommufd -r iommufd_dirty_tracking.domain_dirty128M_huge.enforce_dirty
Thomas
Powered by blists - more mailing lists