linux-kernel - Re: [PATCH] io_thread/x86: don't reset 'cs', 'ss', 'ds' and 'es' registers for io

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <3C41339D-29A2-4AB1-958F-19DB0A92D8D7@amacapital.net>
Date:   Mon, 3 May 2021 09:05:16 -0700
From:   Andy Lutomirski <luto@...capital.net>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Stefan Metzmacher <metze@...ba.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Jens Axboe <axboe@...nel.dk>, linux-kernel@...r.kernel.org,
        io-uring@...r.kernel.org, x86@...nel.org,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH] io_thread/x86: don't reset 'cs', 'ss', 'ds' and 'es' registers for io_threads


> On May 3, 2021, at 7:00 AM, Thomas Gleixner <tglx@...utronix.de> wrote:
> 
> Stefan,
> 
> On Sun, Apr 11 2021 at 17:27, Stefan Metzmacher wrote:
> 
> Can you please CC x86 people on patches which are x86 related?
> 
>> This allows gdb attach to userspace processes using io-uring,
>> which means that they have io_threads (PF_IO_WORKER), which appear
>> just like normal as userspace threads.
> 
> That's not a changelog, really. Please describe what the problem is and
> why the chosen solution is correct.
> 
>> See the code comment for more details.
> 
> The changelog should be self contained.
> 
>> Fixes: 4727dc20e04 ("arch: setup PF_IO_WORKER threads like PF_KTHREAD")
>> Signed-off-by: Stefan Metzmacher <metze@...ba.org>
>> cc: Linus Torvalds <torvalds@...ux-foundation.org>
>> cc: Jens Axboe <axboe@...nel.dk>
>> cc: linux-kernel@...r.kernel.org
>> cc: io-uring@...r.kernel.org
>> ---
>> arch/x86/kernel/process.c | 49 +++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 49 insertions(+)
>> 
>> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
>> index 9c214d7085a4..72120c4b7618 100644
>> --- a/arch/x86/kernel/process.c
>> +++ b/arch/x86/kernel/process.c
>> @@ -163,6 +163,55 @@ int copy_thread(unsigned long clone_flags, unsigned long sp, unsigned long arg,
>>    /* Kernel thread ? */
>>    if (unlikely(p->flags & (PF_KTHREAD | PF_IO_WORKER))) {
>>        memset(childregs, 0, sizeof(struct pt_regs));
>> +        /*
>> +         * gdb sees all userspace threads,
>> +         * including io threads (PF_IO_WORKER)!
>> +         *
>> +         * gdb uses:
>> +         * PTRACE_PEEKUSR, offsetof (struct user_regs_struct, cs)
>> +         *  returning with 0x33 (51) to detect 64 bit
>> +         * and:
>> +         * PTRACE_PEEKUSR, offsetof (struct user_regs_struct, ds)
>> +         *  returning 0x2b (43) to detect 32 bit.
>> +         *
>> +         * GDB relies on that the kernel returns the
>> +         * same values for all threads, which means
>> +         * we don't zero these out.
>> +         *
>> +         * Note that CONFIG_X86_64 handles 'es' and 'ds'
>> +         * differently, see the following above:
>> +         *   savesegment(es, p->thread.es);
>> +         *   savesegment(ds, p->thread.ds);
>> +         * and the CONFIG_X86_64 version of get_segment_reg().
>> +         *
>> +         * Linus proposed something like this:
>> +         * (https://lore.kernel.org/io-uring/CAHk-=whEObPkZBe4766DmR46-=5QTUiatWbSOaD468eTgYc1tg@mail.gmail.com/)
>> +         *
>> +         *   childregs->cs = __USER_CS;
>> +         *   childregs->ss = __USER_DS;
>> +         *   childregs->ds = __USER_DS;
>> +         *   childregs->es = __USER_DS;
>> +         *
>> +         * might make sense (just do it unconditionally, rather than making it
>> +         * special to PF_IO_WORKER).
>> +         *
>> +         * But that doesn't make gdb happy in all cases.
>> +         *
>> +         * While 32bit userspace on a 64bit kernel is legacy,
>> +         * it's still useful to allow 32bit libraries or nss modules
>> +         * use the same code as the 64bit version of that library, which
>> +         * can use io-uring just fine.

Whoa there!  Can we take a big step back?

I saw all the hubbub about making io threads visible to gdb.  Fine, but why do we allow gdb to read and write their register files at all?  They *don’t have user state* because they *are not user threads*.  Beyond that, Linux does not really have a concept of a 32-bit thread and a 64-bit thread. I realize that gdb does have this concept, but gdb is *wrong*, and it regularly causes problems when debugging mixed-mode programs or VMs.

Linus, what is the actual effect of allowing gdb to attach these threads?  Can we instead make all the regset ops do:

if (not actually a user thread) return -EINVAL;

Any other solution results in all kinds of nasty questions. For example, kernel threads don’t have FPU state — what happens if gdb tries to access FPU state?  What happens if gdb tries to *allocate* AMX state for an io_uring thread? What happens if the various remote arch_prctl accessors are used?

—Andy