Message-ID: <871px4cg2x.ffs@tglx>
Date: Wed, 15 Jan 2025 15:22:14 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: reveliofuzzing <reveliofuzzing@...il.com>, Dave Hansen
 <dave.hansen@...el.com>
Cc: mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com,
 kirill.shutemov@...ux.intel.com, x86@...nel.org,
 linux-kernel@...r.kernel.org
Subject: Re: reproducible GPF error in native_tss_update_io_bitmap

On Tue, Jan 07 2025 at 19:24, reveliofuzzing@...il.com wrote:
> On Tue, Jan 7, 2025 at 4:28 PM Dave Hansen <dave.hansen@...el.com> wrote:
>>
>> On 1/6/25 18:32, reveliofuzzing wrote:
>> > Hello,
>> >
>> > We found the following general protection fault bug in Linux kernel 6.12, and
>> > it can be reproduced stably in a QEMU VM. To our knowledge, this problem has not
>> > been observed by SyzBot so we would like to report it for your reference.
>> >
>> > - dmesg
>> > syzkaller login: [   90.849309] Oops: general protection fault,
>> > probably for non-canonical address 0xdffffc0000000000: 0000 [#1]
>> > PREEMPTI
>> > [   90.853735] KASAN: null-ptr-deref in range
>> > [0x0000000000000000-0x0000000000000007]
>> > [   90.856772] CPU: 0 PID: 3265 Comm: iou-sqp-3264 Not tainted 6.10.0 #2
>> > [   90.859386] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
>> > BIOS 1.13.0-1ubuntu1.1 04/01/2014
>> > [   90.862774] RIP: 0010:native_tss_update_io_bitmap+0x143/0x510
>>
>> The whole thing looks like an issue in the failure path when trying to
>> create an io_uring io worker thread. It's probably some confusion in
>> treating the worker thread like a userspace thread with an io bitmap
>> when the worker thread doesn't have one.
>>
>> It's _probably_ only reproducible with io_uring. It's arguable whether
>> it's likely an x86 issue or an io_uring issue.
>>
>> In any case, running:
>>
>>         scripts/decode_stacktrace.sh
>>
> Here is the output of running this script:

So looking at it from the call chain:

> [   90.935319] copy_process (linux-6.12/kernel/fork.c:1764

This means copy_process() failed at some point and then invokes:

> [   90.933995] exit_thread (linux-6.12/arch/x86/kernel/process.c:122)

which in turn invokes:

> [   90.932599] io_bitmap_exit (linux-6.12/arch/x86/kernel/ioport.c:58)

> linux-6.12/arch/x86/kernel/ioport.c:36  - task_update_io_bitmap()
> linux-6.12/arch/x86/kernel/ioport.c:48  - tss_update_io_bitmap()

which ends up in native_tss_update_io_bitmap()

> [   90.853735] KASAN: null-ptr-deref in range
> [0x0000000000000000-0x0000000000000007]
> [   90.856772] CPU: 0 PID: 3265 Comm: iou-sqp-3264 Not tainted 6.10.0 #2
> [   90.859386] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> BIOS 1.13.0-1ubuntu1.1 04/01/2014
> [   90.862774] RIP: 0010:native_tss_update_io_bitmap
> (linux-6.12/arch/x86/kernel/process.c:470)

Which is this code:

	if (tss->io_bitmap.prev_sequence != iobm->sequence)

where @iobm is NULL.

But to reach that code it's required that the current task has
TIF_IO_BITMAP set, which is wrong to begin with.

The context of this is:

[   90.963627]  io_sq_thread+0xaf9/0x1620

which is a IO worker thread created via io_uring_setup(). So that thread
inherits the user space thread TIF flags and indeed the syzkaller
reproducer does:

	syscall(__NR_ioperm, /*from=*/0ul, /*num=*/0xd8ul, /*on=*/0x80000000ul);

before invoking the io_uring setup.

That inheritance is wrong and easy to fix, but that does not explain the
actual failure. That's a bit more subtle.

copy_thread() sets p->thread.io_bitmap to NULL, leaves TIF_IO_BITMAP set
and then returns before sharing the bitmap of the parent thread.

Now copy_process() fails after copy_thread() and invokes exit_thread()
and due to TIF_IO_BITMAP being set it calls io_bitmap_exit(). The latter
invokes task_update_io_bitmap(), which clears the newly created thread's
IO_BITMAP flag because the new thread has neither iopl_emul == 3 nor a
io_valid bitmap.

task_update_io_bitmap() then invokes tss_update_io_bitmap(), which
checks the current thread's TIF_IO_BITMAP bit, which is set, but the
io_sq_thread (current) does neither have iopl_emul == 3 nor a bitmap
pointer. Game over.

Invoking task_update_io_bitmap() in the failure path of copy_process()
is completely wrong as the newly created task never got active and
therefore has never changed the TSS side. So invalidating or updating
anything here is just bogus. The only important part is to drop the
reference count on the bitmap if it got shared in copy_thread().

Tentative uncompiled fix below.

Thanks,

        tglx
---
diff --git a/arch/x86/kernel/ioport.c b/arch/x86/kernel/ioport.c
index e2fab3ceb09f..fa7113babc8e 100644
--- a/arch/x86/kernel/ioport.c
+++ b/arch/x86/kernel/ioport.c
@@ -21,6 +21,9 @@ static atomic64_t io_bitmap_sequence;
 
 void io_bitmap_share(struct task_struct *tsk)
 {
+	/* Inherit the IOPL level of the parent */
+	tsk->thread.iopl_emul = current->thread.iopl_emul;
+
 	/* Can be NULL when current->thread.iopl_emul == 3 */
 	if (current->thread.io_bitmap) {
 		/*
@@ -54,7 +57,12 @@ void io_bitmap_exit(struct task_struct *tsk)
 	struct io_bitmap *iobm = tsk->thread.io_bitmap;
 
 	tsk->thread.io_bitmap = NULL;
-	task_update_io_bitmap(tsk);
+	/*
+	 * Only update when a task is exiting, not when a newly created
+	 * task is mopped up in the failure path of copy_process().
+	 */
+	if (tsk == current)
+		task_update_io_bitmap(tsk);
 	if (iobm && refcount_dec_and_test(&iobm->refcnt))
 		kfree(iobm);
 }
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index f63f8fd00a91..89c16dc135cf 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -176,6 +176,8 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
 	p->thread.sp = (unsigned long) fork_frame;
 	p->thread.io_bitmap = NULL;
 	p->thread.iopl_warn = 0;
+	p->thread.iopl_emul = 0;
+	clear_tsk_thread_flag(p, TIF_IO_BITMAP);
 	memset(p->thread.ptrace_bps, 0, sizeof(p->thread.ptrace_bps));
 
 #ifdef CONFIG_X86_64

Powered by blists - more mailing lists